Potential integer underflow in file suffix generation (valhalla#3783)

* Potential integer underflow in file suffix generation

Integer underflow if maximum tile ID for leven is a 6-digit number.

Signed-off-by: Philipp Voigt (<[email protected]>)

* Potential integer underflow in file suffix generation

Added CHANGELOG.md entry

Signed-off-by: Philipp Voigt (<[email protected]>)

* Potential integer underflow in file suffix generation

Added CHANGELOG.md entry

Signed-off-by: Philipp Voigt (<[email protected]>)

* Potential integer underflow in file suffix generation

Moved CHANGELOG.md entry down

Signed-off-by: Philipp Voigt (<[email protected]>)

* add a test for tile configs with a max tile id that is a multiple of 3 digits. master doesnt fail though

* Make test fail on master

Added test case with GraphId that uses all 6 available digits

Signed-off-by: Philipp Voigt (<[email protected]>)

* Make test fail on master

Confirmed that test fails on master and removed assert

Signed-off-by: Philipp Voigt (<[email protected]>)

* Make test fail on master

Confirmed that test fails on master and removed assert

Signed-off-by: Philipp Voigt (<[email protected]>)

* Update CHANGELOG.md

Signed-off-by: Philipp Voigt (<[email protected]>)
Co-authored-by: Kevin Kreiser <[email protected]>
Co-authored-by: Nils <[email protected]>

Author: 3 people

CHANGELOG.md

@@ -57,6 +57,7 @@
    * FIXED: Fix status endpoint not reporting that the service is shutting down [#3785](https://github.com/valhalla/valhalla/pull/3785)
    * FIXED: Fix TimdDistanceMatrix SetSources and SetTargets [#3792](https://github.com/valhalla/valhalla/pull/3792)
    * FIXED: Added highway and surface factor in truckcost [#3590](https://github.com/valhalla/valhalla/pull/3590)
+   * FIXED: Potential integer underflow in file suffix generation [#3783](https://github.com/valhalla/valhalla/pull/3783)
 
 * **Enhancement**
    * CHANGED: Pronunciation for names and destinations [#3132](https://github.com/valhalla/valhalla/pull/3132)

src/baldr/graphtile.cc

@@ -422,8 +422,10 @@ void GraphTile::AssociateOneStopIds(const GraphId& graphid) {
   }
 }
 
-std::string
-GraphTile::FileSuffix(const GraphId& graphid, const std::string& fname_suffix, bool is_file_path) {
+std::string GraphTile::FileSuffix(const GraphId& graphid,
+                                  const std::string& fname_suffix,
+                                  bool is_file_path,
+                                  const TileLevel* tiles) {
   /*
   if you have a graphid where level == 8 and tileid == 24134109851 you should get:
   8/024/134/109/851.gph since the number of levels is likely to be very small this limits the total
@@ -433,16 +435,18 @@ GraphTile::FileSuffix(const GraphId& graphid, const std::string& fname_suffix, b
   */
 
   // figure the largest id for this level
-  if (graphid.level() >= TileHierarchy::levels().size() &&
-      graphid.level() != TileHierarchy::GetTransitLevel().level) {
+  if ((tiles && tiles->level != graphid.level()) ||
+      (!tiles && graphid.level() >= TileHierarchy::levels().size() &&
+       graphid.level() != TileHierarchy::GetTransitLevel().level)) {
     throw std::runtime_error("Could not compute FileSuffix for GraphId with invalid level: " +
                              std::to_string(graphid));
   }
 
   // get the level info
-  const auto& level = graphid.level() == TileHierarchy::GetTransitLevel().level
-                          ? TileHierarchy::GetTransitLevel()
-                          : TileHierarchy::levels()[graphid.level()];
+  const auto& level = tiles ? *tiles
+                            : (graphid.level() == TileHierarchy::GetTransitLevel().level
+                                   ? TileHierarchy::GetTransitLevel()
+                                   : TileHierarchy::levels()[graphid.level()]);
 
   // figure out how many digits in tile-id
   const auto max_id = level.tiles.ncolumns() * level.tiles.nrows() - 1;
@@ -468,11 +472,11 @@ GraphTile::FileSuffix(const GraphId& graphid, const std::string& fname_suffix, b
   for (uint32_t tile_id = graphid.tileid(); tile_id != 0; tile_id /= 10) {
     tile_id_str[ind--] = '0' + static_cast<char>(tile_id % 10);
     if ((tile_id_strlen - ind) % 4 == 0) {
-      tile_id_str[ind--] = separator;
+      ind--; // skip an additional character to leave space for separators
     }
   }
-  // add missing separators
-  for (size_t sep_ind = 0; sep_ind < ind; sep_ind += 4) {
+  // add separators
+  for (size_t sep_ind = 0; sep_ind < tile_id_strlen; sep_ind += 4) {
     tile_id_str[sep_ind] = separator;
   }
 

test/graphtile.cc

@@ -32,6 +32,12 @@ TEST(Graphtile, FileSuffix) {
   EXPECT_THROW(GraphTile::FileSuffix(GraphId(1036800, 2, 0)), std::runtime_error);
   EXPECT_THROW(GraphTile::FileSuffix(GraphId(4050, 0, 0)), std::runtime_error);
   EXPECT_THROW(GraphTile::FileSuffix(GraphId(1036800, 3, 0)), std::runtime_error);
+
+  TileLevel level{7, valhalla::baldr::RoadClass::kSecondary, "half_degree_is_a_multiple_of_3",
+                  Tiles<PointLL>{{{-180, -90}, {180, 90}}, .5, 1}};
+
+  EXPECT_EQ(GraphTile::FileSuffix(GraphId(1234, 7, 0), ".qux", false, &level), "7/001/234.qux");
+  EXPECT_EQ(GraphTile::FileSuffix(GraphId(123456, 7, 0), ".qux", false, &level), "7/123/456.qux");
 }
 
 TEST(Graphtile, IdFromString) {

valhalla/baldr/graphtile.h

@@ -111,14 +111,17 @@ class GraphTile {
 
   /**
    * Gets the directory like filename suffix given the graphId
-   * @param  graphid  Graph Id to construct filename.
-   * @param  gzipped  Modifies the suffix if you expect gzipped file names
+   * @param  graphid      Graph Id to construct filename.
+   * @param  gzipped      Modifies the suffix if you expect gzipped file names
    * @param  is_file_path Determines the 1000 separator to be used for file or URL access
+   * @param  tiles        Allows passing a custom tile definition rather than pulling from static
+   *                      hierarchy, which is useful for testing
    * @return  Returns a filename including directory path as a suffix to be appended to another uri
    */
   static std::string FileSuffix(const GraphId& graphid,
                                 const std::string& suffix = valhalla::baldr::SUFFIX_NON_COMPRESSED,
-                                bool is_file_path = true);
+                                bool is_file_path = true,
+                                const TileLevel* tiles = nullptr);
 
   /**
    * Get the tile Id given the full path to the file.