Overflow in pxy2zxy

[springmeyer]

By running carmen-cache with -fsanitze=integer I see we only have one test that hits the if (zDist == 0) condition at

carmen-cache/src/binding.cpp

Line 934 in cd20c47

if (zDist == 0) {

.

It is the test/geocode-unit.proximity.test.js (specifically # forward province - multilayer) in carmen as of https://github.com/mapbox/carmen/commit/dbe167d609a6cc5374a20ddd7accb81ba62edf34.

Before the if (zDist == 0) condition the zMult variable overflows. This is harmless because it is not used, but it still ends up being 4294967295 because target_z and z are both 6 and so zDist because 0 at

carmen-cache/src/binding.cpp

Line 932 in cd20c47

unsigned zDist = target_z - z;

Tasks:

• Ensure we have test coverage in carmen-cache directly that triggers this condition (to protect from a regression if the if (zDist == 0) condition were every accidentally removed
• Fix the harmless overflow

Proposed fix:

diff --git a/src/binding.cpp b/src/binding.cpp
index b6bb856..a424111b 100644
--- a/src/binding.cpp
+++ b/src/binding.cpp
@@ -903,12 +903,12 @@ ZXY pxy2zxy(unsigned z, unsigned x, unsigned y, unsigned target_z) {
 
     // Interval between parent and target zoom level
     unsigned zDist = target_z - z;
-    unsigned zMult = zDist - 1;
     if (zDist == 0) {
         zxy.x = x;
         zxy.y = y;
         return zxy;
     }
+    unsigned zMult = zDist - 1;
 
     // Midpoint length @ z for a tile at parent zoom level
     unsigned pMid_d = static_cast<unsigned>(std::pow(2,zDist) / 2);

[springmeyer]

Just tested again and I'm also seeing a similar potential overflow in bxy2zxy (

carmen-cache/src/binding.cpp

Line 956 in 64af63c

signed zDist = target_z - z;

)

# coalesceMulti bbox
../src/binding.cpp:956:29: runtime error: unsigned integer overflow: 1 - 2 cannot be represented in type 'unsigned int'
SUMMARY: AddressSanitizer: undefined-behavior ../src/binding.cpp:956:29 in 

[springmeyer]

Note: these problems are still present. They are minor, but should ideally still be fixed. They are not seen in the carmen-cache tests because of lacking coverage. They only show up when carmen-cache is instrumented with the address sanitizer and the carmen tests are run with this version of carmen-cache.

I don't have the bandwidth to keep pushing here, so I will lay out the remaining todos and offer to spirit guide someone who picks this up:

• merge Build and test against clang++ sanitizers on travis #95 will starts publishing sanitized binaries for linux
• optionally: test those binaries downstream in carmen (npm install --toolset=asan will install them)
• See if the overflow errors still exits
• Add unit tests in carmen-cache that catch them, then the tests should start failing in carmen-cache
• Fix the overflows

[springmeyer]

presume this was fixed by #116