and Fixing Null Pointer Subtraction in SQLite SHA3 Extension

Issue Overview: Null Pointer Subtraction in SHA3 Extension Code

The core issue revolves around a specific line of code in the SQLite SHA3 extension (shathree.c), where a null pointer subtraction is performed. The code in question is part of a conditional check that determines whether the pointer aData is aligned to an 8-byte boundary. The original code uses a subtraction of a null pointer to achieve this alignment check, which triggers a compiler warning due to undefined behavior. The warning is generated because subtracting a null pointer from another pointer is not well-defined in the C standard, even though the intent is to check the alignment of the pointer.

The original code snippet is as follows:

if( (p->nLoaded % 8)==0 && ((aData - (const unsigned char*)0)&7)==0 ){

The warning message from the compiler is:

warning: performing pointer subtraction with a null pointer has undefined behavior [-Wnull-pointer-subtraction]

The goal is to understand why this subtraction by zero exists, whether it is safe to remove it, and what the best alternative implementation would be to achieve the same functionality without triggering compiler warnings or introducing undefined behavior.

Possible Causes: Why Null Pointer Subtraction Was Used

The use of null pointer subtraction in the original code is a historical artifact and a clever (but non-standard) trick to determine the alignment of a pointer. The intent of the code is to check whether the pointer aData is aligned to an 8-byte boundary. This is important for performance reasons, as certain operations (e.g., memory access, cryptographic computations) are more efficient when data is aligned to specific boundaries.

The expression ((aData - (const unsigned char*)0) & 7) == 0 is designed to check the alignment of aData. Here’s how it works:

1. (const unsigned char*)0 casts the null pointer to a const unsigned char* type.
2. Subtracting this null pointer from aData effectively calculates the offset of aData from the null pointer. This offset is equivalent to the numerical value of the pointer aData itself.
3. The result of the subtraction is then bitwise ANDed with 7 (& 7). If the result is 0, it means the lowest 3 bits of the pointer are 0, indicating that the pointer is aligned to an 8-byte boundary.

While this trick works on most platforms and compilers, it relies on undefined behavior according to the C standard. Specifically, the C standard does not define the behavior of pointer arithmetic involving null pointers. This is why modern compilers issue warnings when encountering such code.

The use of this trick likely stems from a desire to write concise and efficient code without introducing additional variables or casts. However, as compilers have become more strict about undefined behavior, this approach has become less acceptable.

Troubleshooting Steps, Solutions & Fixes: Modernizing the Alignment Check

To resolve the issue and eliminate the compiler warning, the code must be rewritten to avoid null pointer subtraction while still achieving the same functionality. There are several approaches to achieve this, each with its own trade-offs in terms of readability, portability, and adherence to the C standard.

Solution 1: Casting the Pointer to size_t

The simplest and most portable solution is to cast the pointer aData to size_t, which is an unsigned integer type guaranteed to be large enough to hold the value of a pointer. This approach avoids undefined behavior and is compatible with all platforms supported by SQLite.

The modified code would look like this:

if( (p->nLoaded % 8)==0 && (((size_t)aData) & 7)==0 ){

This solution works because:

1. Casting the pointer to size_t converts the pointer to its numerical representation.
2. The bitwise AND operation (& 7) checks the lowest 3 bits of the pointer, which must be 0 for the pointer to be aligned to an 8-byte boundary.

This approach is both standard-compliant and easy to understand. It is also consistent with the C89 standard, which is a requirement for SQLite.

Solution 2: Using intptr_t for Clarity

Another approach is to use intptr_t, which is an integer type specifically designed to hold a pointer value. This type was introduced in C99 and provides a clearer indication of the intent to work with pointer values as integers.

The modified code would look like this:

if( (p->nLoaded % 8)==0 && (((intptr_t)aData) & 7)==0 ){

This solution is functionally equivalent to the size_t approach but is slightly more explicit about the intent to treat the pointer as an integer. However, it requires C99 compliance, which may not be desirable in all contexts.

Solution 3: Avoiding Pointer Arithmetic Altogether

If the goal is to avoid any reliance on pointer arithmetic or casting, an alternative approach is to use a union to access the pointer’s bits directly. This method is more verbose but avoids any potential issues with pointer arithmetic.

The modified code would look like this:

union {
    const unsigned char *ptr;
    size_t value;
} u;
u.ptr = aData;
if( (p->nLoaded % 8)==0 && (u.value & 7)==0 ){

This approach uses a union to store the pointer and access its numerical representation without performing any pointer arithmetic. While this method is safe and portable, it is more complex and less readable than the previous solutions.

Solution 4: Using Compiler-Specific Attributes

Some compilers provide attributes or built-in functions to check pointer alignment. For example, GCC and Clang provide the __builtin_alignof and __builtin_is_aligned functions, which can be used to check alignment without resorting to pointer arithmetic.

The modified code would look like this:

if( (p->nLoaded % 8)==0 && __builtin_is_aligned(aData, 8) ){

This approach is highly readable and avoids any undefined behavior. However, it is not portable and relies on compiler-specific features, which may not be suitable for all projects.

Final Recommendation

The best solution depends on the specific requirements of the project. For SQLite, which prioritizes portability and adherence to the C89 standard, the size_t cast is the most appropriate choice. It is simple, portable, and avoids undefined behavior while achieving the desired functionality.

The final recommended code is:

if( (p->nLoaded % 8)==0 && (((size_t)aData) & 7)==0 ){

This solution resolves the compiler warning, adheres to the C standard, and maintains the original functionality of the code. It is also consistent with the SQLite project’s emphasis on simplicity and portability.