Changeset 19033 in josm

Timestamp:

2024-04-05T14:58:43+02:00 (5 weeks ago)

Author:

GerdP

Message:

fix #23599: Error opening osm.pbf if way has negative ID or incomplete metadata
This fixes

• handling of visible flag as proposed by *Martin*
• handling of completely missing denseinfo (ids are always delta encoded), upload is discouraged
• handling of denseinfo which only contains version data

Location:

trunk

Files:

• src/org/openstreetmap/josm/io/OsmPbfReader.java (modified) (11 diffs)
• test/data/regress/23599 (added)
• test/data/regress/23599/visible.osm.pbf (added)
• test/data/regress/23599/w1194668585.drop-author.osm.pbf (added)
• test/data/regress/23599/w1194668585.drop-version.osm.pbf (added)
• test/data/regress/23599/w1194668585.full.osm.pbf (added)
• test/data/regress/23599/w1194668585.orig.osm (added)
• test/unit/org/openstreetmap/josm/gui/io/importexport/OsmPbfImporterTest.java (modified) (2 diffs)

trunk/src/org/openstreetmap/josm/io/OsmPbfReader.java

@@ -16 +16 @@
 import java.util.Set;
 
-import jakarta.annotation.Nonnull;
-import jakarta.annotation.Nullable;
-
 import org.apache.commons.compress.utils.CountingInputStream;
 import org.openstreetmap.josm.data.Bounds;
@@ -31 +28 @@
 import org.openstreetmap.josm.data.osm.RelationMemberData;
 import org.openstreetmap.josm.data.osm.Tagged;
+import org.openstreetmap.josm.data.osm.UploadPolicy;
 import org.openstreetmap.josm.data.osm.User;
 import org.openstreetmap.josm.data.osm.WayData;
@@ -44 +42 @@
 import org.openstreetmap.josm.gui.progress.ProgressMonitor;
 import org.openstreetmap.josm.tools.Utils;
+
+import jakarta.annotation.Nonnull;
+import jakarta.annotation.Nullable;
 
 /**
@@ -523 +524 @@
             if (info != null) {
                 setOsmPrimitiveData(primitiveBlockRecord, node, info);
+            } else {
+                ds.setUploadPolicy(UploadPolicy.DISCOURAGED);
             }
             buildPrimitive(node);
@@ -572 +575 @@
             }
         }
+
         int keyValIndex = 0; // This index must not reset between nodes, and must always increment
         if (ids.length == lats.length && lats.length == lons.length && (denseInfo == null || denseInfo.length == lons.length)) {
@@ -579 +583 @@
             for (int i = 0; i < ids.length; i++) {
                 final NodeData node;
+                id += ids[i];
+                node = new NodeData(id);
                 if (denseInfo != null) {
                     Info info = denseInfo[i];
-                    id += ids[i];
-                    node = new NodeData(id);
                     setOsmPrimitiveData(primitiveBlockRecord, node, info);
                 } else {
-                    node = new NodeData(ids[i]);
+                    ds.setUploadPolicy(UploadPolicy.DISCOURAGED);
                 }
                 lat += lats[i];
@@ -679 +683 @@
         if (info != null) {
             setOsmPrimitiveData(primitiveBlockRecord, wayData, info);
+        } else {
+            ds.setUploadPolicy(UploadPolicy.DISCOURAGED);
         }
         buildPrimitive(wayData);
@@ -744 +750 @@
         if (info != null) {
             setOsmPrimitiveData(primitiveBlockRecord, data, info);
+        } else {
+            ds.setUploadPolicy(UploadPolicy.DISCOURAGED);
         }
         addTags(data, keys, values);
@@ -796 +804 @@
                         break;
                     case 6:
-                        visible = protobufRecord.asUnsignedVarInt().byteValue() == 0;
+                        visible = protobufRecord.asUnsignedVarInt().byteValue() == 1;
                         break;
                     default: // Fall through, since the PBF format could be extended
@@ -947 +955 @@
             }
         }
-        if (version.length == timestamp.length && timestamp.length == changeset.length && changeset.length == uid.length &&
-                uid.length == userSid.length && (visible == EMPTY_LONG || visible.length == userSid.length)) {
+        if (version.length > 0) {
             Info[] infos = new Info[version.length];
             long lastTimestamp = 0; // delta encoded
@@ -955 +962 @@
             long lastUserSid = 0; // delta encoded, string id for username
             for (int i = 0; i < version.length; i++) {
-                lastTimestamp += timestamp[i];
-                lastChangeset += changeset[i];
-                lastUid += uid[i];
-                lastUserSid += userSid[i];
+                if (timestamp.length > i)
+                    lastTimestamp += timestamp[i];
+                if (changeset.length > i)
+                    lastChangeset += changeset[i];
+                if (uid.length > i && userSid.length > i) {
+                    lastUid += uid[i];
+                    lastUserSid += userSid[i];
+                }
                 infos[i] = new Info((int) version[i], lastTimestamp, lastChangeset, (int) lastUid, (int) lastUserSid,
                         visible == EMPTY_LONG || visible[i] == 1);

trunk/test/unit/org/openstreetmap/josm/gui/io/importexport/OsmPbfImporterTest.java

@@ -24 +24 @@
 import org.openstreetmap.josm.data.osm.AbstractPrimitive;
 import org.openstreetmap.josm.data.osm.DataSet;
+import org.openstreetmap.josm.data.osm.OsmPrimitive;
 import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
 import org.openstreetmap.josm.data.osm.Relation;
+import org.openstreetmap.josm.data.osm.UploadPolicy;
 import org.openstreetmap.josm.data.osm.Way;
 import org.openstreetmap.josm.data.protobuf.ProtobufTest;
@@ -159 +161 @@
         assertDoesNotThrow(() -> importer.parseDataSet(new ByteArrayInputStream(badData), NullProgressMonitor.INSTANCE));
     }
+
+    @Test
+    void testNonRegression23599a() throws IOException, IllegalDataException {
+        final DataSet dataSet;
+        try (InputStream inputStream = TestUtils.getRegressionDataStream(23599, "visible.osm.pbf")) {
+            dataSet = importer.parseDataSet(inputStream, NullProgressMonitor.INSTANCE);
+        }
+        assertTrue(dataSet.getNodes().stream().allMatch(OsmPrimitive::isVisible));
+        assertTrue(dataSet.getWays().stream().allMatch(OsmPrimitive::isVisible));
+        assertTrue(dataSet.getRelations().stream().allMatch(OsmPrimitive::isVisible));
+
+    }
+
+    @Test
+    void testNonRegression23599b() throws IOException, IllegalDataException {
+        final DataSet dataSet;
+        // osmconvert w1194668585.orig.osm -o=w1194668585.full.osm.pbf
+        try (InputStream inputStream = TestUtils.getRegressionDataStream(23599, "w1194668585.full.osm.pbf")) {
+            dataSet = importer.parseDataSet(inputStream, NullProgressMonitor.INSTANCE);
+        }
+        assertNotNull(dataSet.getPrimitiveById(1194668585L, OsmPrimitiveType.WAY));
+        assertEquals(145987123, dataSet.getPrimitiveById(1194668585L, OsmPrimitiveType.WAY).getChangesetId());
+        assertEquals(4, dataSet.getPrimitiveById(1194668585L, OsmPrimitiveType.WAY).getVersion());
+        assertEquals(UploadPolicy.NORMAL, dataSet.getUploadPolicy());
+    }
+
+    @Test
+    void testNonRegression23599c() throws IOException, IllegalDataException {
+        final DataSet dataSet;
+        // osmconvert --drop-author w1194668585.orig.osm -o=w1194668585.full.osm.pbf
+        try (InputStream inputStream = TestUtils.getRegressionDataStream(23599, "w1194668585.drop-author.osm.pbf")) {
+            dataSet = importer.parseDataSet(inputStream, NullProgressMonitor.INSTANCE);
+        }
+        assertNotNull(dataSet.getPrimitiveById(1194668585L, OsmPrimitiveType.WAY));
+        assertEquals(0, dataSet.getPrimitiveById(1194668585L, OsmPrimitiveType.WAY).getChangesetId());
+        assertEquals(4, dataSet.getPrimitiveById(1194668585L, OsmPrimitiveType.WAY).getVersion());
+        assertEquals(UploadPolicy.NORMAL, dataSet.getUploadPolicy());
+    }
+
+    @Test
+    void testNonRegression23599d() throws IOException, IllegalDataException {
+        final DataSet dataSet;
+        // osmconvert --drop-version w1194668585.orig.osm -o=w1194668585.full.osm.pbf
+        try (InputStream inputStream = TestUtils.getRegressionDataStream(23599, "w1194668585.drop-version.osm.pbf")) {
+            dataSet = importer.parseDataSet(inputStream, NullProgressMonitor.INSTANCE);
+        }
+        assertNotNull(dataSet.getPrimitiveById(1194668585L, OsmPrimitiveType.WAY));
+        assertEquals(0, dataSet.getPrimitiveById(1194668585L, OsmPrimitiveType.WAY).getChangesetId());
+        assertEquals(0, dataSet.getPrimitiveById(1194668585L, OsmPrimitiveType.WAY).getVersion());
+        assertEquals(UploadPolicy.DISCOURAGED, dataSet.getUploadPolicy());
+    }
+
 }