[PATCH v2 4/4] obstack: avoid computing offsets from NULL pointer

[Jeff King <peff@peff.net>]

On Sat, Jan 25, 2020 at 12:41:18AM -0500, Jeff King wrote:

> diff --git a/compat/obstack.h b/compat/obstack.h
> index 01e7c81840..fbcf68c67c 100644
> --- a/compat/obstack.h
> +++ b/compat/obstack.h
> @@ -135,8 +135,10 @@ extern "C" {
>     alignment relative to 0.  */
>  
>  #define __PTR_ALIGN(B, P, A)						    \
> -  __BPTR_ALIGN (sizeof (PTR_INT_TYPE) < sizeof (void *) ? (B) : (char *) 0, \
> -		P, A)
> +  (sizeof (PTR_INT_TYPE) < sizeof(void *) ?                                 \
> +   __BPTR_ALIGN((B), (P), (A)) :                                            \
> +   (void *)__BPTR_ALIGN((PTR_INT_TYPE)0, (PTR_INT_TYPE)(P), (A))            \
> +  )

Oops, I added in one change at the last minute but failed to commit it.
The first argument to __BPTR_ALIGN() should be:

  (PTR_INT_TYPE)(void *)0

to ensure that it's a NULL pointer, even if the platform doesn't have
an all-bits-zero NULL. Again, this is probably academic for Git's
codebase, but I was trying to keep as close to the original as possible.

Here's a fixed patch.

-- >8 --
Subject: obstack: avoid computing offsets from NULL pointer

As with the previous two commits, UBSan with clang-11 complains about
computing offsets from a NULL pointer. The failures in t4013 (and
elsewhere) look like this:

  kwset.c:102:23: runtime error: applying non-zero offset 107820859019600 to null pointer
  ...
  not ok 79 - git log -SF master # magic is (not used)

That line is not enlightening:

  ... = obstack_alloc(&kwset->obstack, sizeof (struct trie));

because obstack is implemented almost entirely in macros, and the actual
problem is five macros deep (I temporarily converted them to inline
functions to get better compiler errors, which was tedious but worked
reasonably well).

The actual problem is in these pointer-alignment macros:

  /* If B is the base of an object addressed by P, return the result of
     aligning P to the next multiple of A + 1.  B and P must be of type
     char *.  A + 1 must be a power of 2.  */

  #define __BPTR_ALIGN(B, P, A) ((B) + (((P) - (B) + (A)) & ~(A)))

  /* Similar to _BPTR_ALIGN (B, P, A), except optimize the common case
     where pointers can be converted to integers, aligned as integers,
     and converted back again.  If PTR_INT_TYPE is narrower than a
     pointer (e.g., the AS/400), play it safe and compute the alignment
     relative to B.  Otherwise, use the faster strategy of computing the
     alignment relative to 0.  */

  #define __PTR_ALIGN(B, P, A)                                                \
    __BPTR_ALIGN (sizeof (PTR_INT_TYPE) < sizeof (void *) ? (B) : (char *) 0, \
                  P, A)

If we have a sufficiently-large integer pointer type, then we do the
computation using a NULL pointer constant. That turns __BPTR_ALIGN()
into something like:

  NULL + (P - NULL + A) & ~A

and UBSan is complaining about adding the full value of P to that
initial NULL. We can fix this by doing our math as an integer type, and
then casting the result back to a pointer. The problem case only happens
when we know that the integer type is large enough, so there should be
no issue with truncation.

Another option would be just simplify out all the 0's from
__BPTR_ALIGN() for the NULL-pointer case. That probably wouldn't work
for a platform where the NULL pointer isn't all-zeroes, but Git already
wouldn't work on such a platform (due to our use of memset to set
pointers in structs to NULL). But I tried here to keep as close to the
original as possible.

Signed-off-by: Jeff King <peff@peff.net>
---
 compat/obstack.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/compat/obstack.h b/compat/obstack.h
index 01e7c81840..f90a46d9b9 100644
--- a/compat/obstack.h
+++ b/compat/obstack.h
@@ -135,8 +135,10 @@ extern "C" {
    alignment relative to 0.  */
 
 #define __PTR_ALIGN(B, P, A)						    \
-  __BPTR_ALIGN (sizeof (PTR_INT_TYPE) < sizeof (void *) ? (B) : (char *) 0, \
-		P, A)
+  (sizeof (PTR_INT_TYPE) < sizeof(void *) ?                                 \
+   __BPTR_ALIGN((B), (P), (A)) :                                            \
+   (void *)__BPTR_ALIGN((PTR_INT_TYPE)(void *)0, (PTR_INT_TYPE)(P), (A))            \
+  )
 
 #include <string.h>
 
-- 
2.25.0.421.gb74d19af79