poly_r.c: fix crash when building with -ftrapv on 32 bit builds

[rouault]

This is a upstreaming from GDAL's internal libqhull copy:
OSGeo/gdal@3c2f7d2

[stevengj]

This doesn't seem right — I think for hashing you actually want the 2s-complement wraparound behavior?

It seems like a better fix would be to define a ptr_uintT unsigned type (= uintptr_t in C99) and use that here, since -ftrapv only checks for signed-integer overflow.

[cbbarber]

ptr_intT is a Qhull typedef defined in mem_r.h

Please submit a new pull request to change ptr_intT to intptr_t for C99+ compilers. Is another 'include' required for intptr_t?

typedef intptr_t ptr_intT;

Otherwise keep the current definition. It is

#if (defined(MINGW64)) && defined(_WIN64)
typedef long long ptr_intT;
#elif defined(_MSC_VER) && defined(_WIN64)
typedef long long ptr_intT;
#else
typedef long ptr_intT;
#endif

Note that Steven suggested 'uintptr_t' instead of 'intptr_t'. It would be a better choice for 'gethash' in poly_r.h, but it would require another critical typedef and a rewrite of gethash.

The code is admittedly odd. I'll add a review for a later release of Qhull. Perhaps int_ptrT should be changed to uint_ptrT everywhere.

[rouault]

I don't think changing to intptr_t would solve the issue on 32 bit builds, since it is still a signed type and the overflow on addition would still occur. The temporary cast to long long allows arithmetics to be done on 64 bits before the truncation (which is also probably undefined behaviour formally too, but -ftrapv and -fsanitize=undefined don't catch this). Which underlies that if there was a 64 bit architecture using the full width of 64 bits for address, we could have issues there too, but the x64 convention is limited to 48 bits, so it doesn't happen here.
Changing to unsigned type would probably be better, but I'm not knowledgeable enough of the code base to assess implications.

[cbbarber]

Postponing this request to a reorg of Qhull for 64-bit code.

A higher priority task is merge due to 'dupridge'.

[thurstond]

The temporary cast to long long allows arithmetics to be done on 64 bits before the truncation (which is also probably undefined behaviour formally too, but -ftrapv and -fsanitize=undefined don't catch this). Which underlies that if there was a 64 bit architecture using the full width of 64 bits for address, we could have issues there too, but the x64 convention is limited to 48 bits, so it doesn't happen here. Changing to unsigned type would probably be better, but I'm not knowledgeable enough of the code base to assess implications.

HardwareAddressSanitizer [1] tags the top-byte of the pointer, which often leads to signed integer overflow in that hashing function. Changing qh_gethash to temporarily cast to unsigned long long:

   case 1:
-    hash= (ptr_intT)(*elemp) - (ptr_intT) skipelem;
+    hash= (ptr_uintT)(*elemp) - (ptr_uintT) skipelem; // Added unsigned typedef in mem_r.h
     break;
    // Similarly for other cases

fixes the issue, while keeping the change localized (the type of ptr_intT isn't changed elsewhere), thus ensuring it won't adversely affect the rest of the code.

[1] https://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html

[cbbarber]

thurstond - good argument. Can you create the corresponding patch?

1. Define ptr_uintT immediately after ptr_intT in user_r.h and user.h
2. Add ptr_uintT to qh-mem.htm immediately after ptr_intT
3. Change qh_gethash to use ptr_uintT. The destination should be 'uresult' instead of 'hash'

The only other use of ptr_intT is qh_pointid in poly_r.c and poly.c.. This use looks OK. Agreed?
offset= (ptr_intT)(point - qh->first_point);