Edited OSM XML crashes the scanner

[culebron-ala]

If I edit an OSM XML file in JOSM, save it and run the parser on it, it crashes.

The error is caused when something inside meets a negative node ID and panics.

I specially tested it, and when I took the modified file, that caused the crash, and in a text editor, changed the negative ID to positive, everything worked well.

My understanding is that there's nothing wrong editing the files and not uploading the changes, and having negative ids. Also, NodeID is int64, not uint64.

Notice that the error is itself also misleading: it happens when I call oid.Type(), not when fetching the object from the scanner.

Code to demonstrate the problem:

package main

import (
	"context"
	"fmt"
	"os"

	"github.com/paulmach/osm"
	"github.com/paulmach/osm/osmxml"
)

func scan_osm(path string) {
	f, _ := os.Open(path)
	scanner := osmxml.New(context.Background(), f)
	fmt.Printf("\n\nScanning %s =======================\n", path)
	for scanner.Scan() {
		o := scanner.Object()
		oid := o.ObjectID()
		if oid.Type() == "way" {
			way := o.(*osm.Way)
			fmt.Println(way)
		}

	}
}
func main() {
	scan_osm("ok.osm")
	scan_osm("fixed.osm")
	scan_osm("error.osm")
	fmt.Println("everything worked well")
}

ok.osm

<?xml version='1.0' encoding='UTF-8'?>
<osm version='0.6' generator='JOSM'>
  <node id='2503966846' timestamp='2015-09-29T18:24:39Z' uid='1772173' user='plguedes' visible='true' version='2' changeset='34332332' lat='-8.0769279' lon='-34.8973969' />
  <node id='2503966854' timestamp='2020-02-26T15:56:11Z' uid='9535235' user='Metaxourgeio' visible='true' version='4' changeset='81511274' lat='-8.0776001' lon='-34.8971701' />
  <way id='242959928' timestamp='2024-10-17T23:06:04Z' uid='4102129' user='raphaelmirc' visible='true' version='6' changeset='158029161'>
    <nd ref='2503966854' />
    <nd ref='2503966846' />
    <tag k='highway' v='residential' />
    <tag k='name' v='Rua Pacatuba' />
    <tag k='oneway' v='yes' />
    <tag k='postal_code' v='50090-570' />
    <tag k='source:postal_code' v='CNEFE' />
    <tag k='surface' v='asphalt' />
  </way>
</osm>

fixed.osm (I changed the ID to positive, and it fixed the error)

 <?xml version='1.0' encoding='UTF-8'?>
<osm version='0.6' generator='JOSM'>
  <node id='25490' action='modify' visible='true' lat='-8.07731878077' lon='-34.89726501704' />
  <node id='2503966846' timestamp='2015-09-29T18:24:39Z' uid='1772173' user='plguedes' visible='true' version='2' changeset='34332332' lat='-8.0769279' lon='-34.8973969' />
  <node id='2503966854' timestamp='2020-02-26T15:56:11Z' uid='9535235' user='Metaxourgeio' visible='true' version='4' changeset='81511274' lat='-8.0776001' lon='-34.8971701' />
  <way id='242959928' action='modify' timestamp='2024-10-17T23:06:04Z' uid='4102129' user='raphaelmirc' visible='true' version='6' changeset='158029161'>
    <nd ref='2503966854' />
    <nd ref='25490' />
    <nd ref='2503966846' />
    <tag k='highway' v='residential' />
    <tag k='name' v='Rua Pacatuba' />
    <tag k='oneway' v='yes' />
    <tag k='postal_code' v='50090-570' />
    <tag k='source:postal_code' v='CNEFE' />
    <tag k='surface' v='asphalt' />
  </way>
</osm>

error.osm (produced by JOSM)

<?xml version='1.0' encoding='UTF-8'?>
<osm version='0.6' generator='JOSM'>
  <node id='-25490' action='modify' visible='true' lat='-8.07731878077' lon='-34.89726501704' />
  <node id='2503966846' timestamp='2015-09-29T18:24:39Z' uid='1772173' user='plguedes' visible='true' version='2' changeset='34332332' lat='-8.0769279' lon='-34.8973969' />
  <node id='2503966854' timestamp='2020-02-26T15:56:11Z' uid='9535235' user='Metaxourgeio' visible='true' version='4' changeset='81511274' lat='-8.0776001' lon='-34.8971701' />
  <way id='242959928' action='modify' timestamp='2024-10-17T23:06:04Z' uid='4102129' user='raphaelmirc' visible='true' version='6' changeset='158029161'>
    <nd ref='2503966854' />
    <nd ref='-25490' />
    <nd ref='2503966846' />
    <tag k='highway' v='residential' />
    <tag k='name' v='Rua Pacatuba' />
    <tag k='oneway' v='yes' />
    <tag k='postal_code' v='50090-570' />
    <tag k='source:postal_code' v='CNEFE' />
    <tag k='surface' v='asphalt' />
  </way>
</osm>

Output of the code:

Scanning ok.osm =======================
&{{ } 242959928 raphaelmirc 4102129 true 6 158029161 2024-10-17 23:06:04 +0000 UTC [{2503966854 0 0 0 0} {2503966846 0 0 0 0}] [{highway residential} {name Rua Pacatuba} {oneway yes} {postal_code 50090-570} {source:postal_code CNEFE} {surface asphalt}] <nil> [] <nil>}


Scanning fixed.osm =======================
&{{ } 242959928 raphaelmirc 4102129 true 6 158029161 2024-10-17 23:06:04 +0000 UTC [{2503966854 0 0 0 0} {25490 0 0 0 0} {2503966846 0 0 0 0}] [{highway residential} {name Rua Pacatuba} {oneway yes} {postal_code 50090-570} {source:postal_code CNEFE} {surface asphalt}] <nil> [] <nil>}


Scanning error.osm =======================
panic: unknown type

goroutine 1 [running]:
github.com/paulmach/osm.ObjectID.Type(...)
        ..../go/pkg/mod/github.com/paulmach/osm@v0.8.0/object.go:32
main.scan_osm({0x1027a0829, 0x9})
        ..../Documents/teststand/demo/main.go:19 +0x244
main.main()
        ..../Documents/teststand/demo/main.go:29 +0x48
exit status 2

[culebron-ala]

I checked, and it's indeed the negative ID. Just these 1 tag is enough:

fixed.osm

<?xml version='1.0' encoding='UTF-8'?>
<osm version='0.6' generator='JOSM' id='-456'>
  <node id='25490' action='modify' visible='true' lat='-8.07731878077' lon='-34.89726501704' />
</osm>

error.osm

<?xml version='1.0' encoding='UTF-8'?>
<osm version='0.6' generator='JOSM' id='-123'>
  <node id='-25490' action='modify' visible='true' lat='-8.07731878077' lon='-34.89726501704' />
</osm>

The returned object becomes of panic type, and the code panics if you touch anything in it.

I went through the code with a debugger, and I can't see anything that assumes unsigned int ID. Maybe it's the XML spec & xml package itself?

[culebron-ala]

I see in this line the code calls the decoder, and probably there inside the panic happens.
https://github.com/paulmach/osm/blob/master/osmxml/scanner.go#L92

[culebron-ala]

No, standard xml parser doesn't break anything.
Walking with a debugger, I see this line tries to filter the ID with bitmask, and instead of node, we get to the panic call.
https://github.com/paulmach/osm/blob/master/object.go#L15

[culebron-ala]

I guess trying to encode and shift bits is what breaks the negative IDs.
https://github.com/paulmach/osm/blob/master/object.go#L37

[culebron-ala]

I took scanner.go and used it in my lib, in a patched version, bypassing this bit shift magic. Since we can't have enums with nested values, I just put an enum nearby the object.

package main

import (
	"context"
	"encoding/xml"
	"io"
	"strings"

	"github.com/paulmach/osm"
)

type ObjectKind uint8

const (
	ONode ObjectKind = iota
	OWay
	ORelation
)

type ParsedObject struct {
	Obj  osm.Object
	Kind ObjectKind
}

type Scanner struct {
	ctx    context.Context
	done   context.CancelFunc
	closed bool

	decoder *xml.Decoder
	next    *ParsedObject
	err     error
}

// New returns a new Scanner to read from r.
func NewScanner(ctx context.Context, r io.Reader) *Scanner {
	if ctx == nil {
		ctx = context.Background()
	}

	s := &Scanner{
		decoder: xml.NewDecoder(r),
	}

	s.ctx, s.done = context.WithCancel(ctx)
	return s
}

func (s *Scanner) Close() error {
	s.closed = true
	s.done()

	return nil
}

func (s *Scanner) Scan() bool {
	if s.err != nil {
		return false
	}

Loop:
	for {
		if s.ctx.Err() != nil {
			return false
		}

		t, err := s.decoder.Token()
		if err != nil {
			s.err = err
			return false
		}

		se, ok := t.(xml.StartElement)
		if !ok {
			continue
		}

		s.next = nil
		switch strings.ToLower(se.Name.Local) {
		case "node":
			node := &osm.Node{}
			err = s.decoder.DecodeElement(&node, &se)
			s.next = &ParsedObject{Obj: node, Kind: ONode}
		case "way":
			way := &osm.Way{}
			err = s.decoder.DecodeElement(&way, &se)
			s.next = &ParsedObject{Obj: way, Kind: OWay}
		case "relation":
			relation := &osm.Relation{}
			err = s.decoder.DecodeElement(&relation, &se)
			s.next = &ParsedObject{Obj: relation, Kind: ORelation}
		default:
			continue Loop
		}

		if err != nil {
			s.err = err
			return false
		}

		return true
	}
}

func (s *Scanner) Object() *ParsedObject {
	return s.next
}

// Err returns the first non-EOF error that was encountered by the Scanner.
func (s *Scanner) Err() error {
	if s.err == io.EOF {
		return nil
	}

	if s.err != nil {
		return s.err
	}

	if s.closed {
		return osm.ErrScannerClosed
	}

	return s.ctx.Err()
}

Usage:

		o := scanner.Object()

		if o.Kind != OWay {
			continue
		}

		way := o.Obj.(*osm.Way)

[paulmach]

The issue is the libray doesn't/didn't support negative ids. I have PR to support them here #64

[culebron-ala]

Thanks a lot for such quick fix! I wasn't expecting this.