1578626 - [CID 1453375] UB: decrement nullptr.

Status: RESOLVED FIXED

(NSS :: Test, defect)

Description

[Martin Thomson [:mt:]]

 *** CID 1453375:  Null pointer dereferences  (FORWARD_NULL)
/gtests/mozpkix_gtest/pkixder_input_tests.cpp: 256 in <unnamed>::pkixder_input_tests_ReadWordWrapAroundPointer_Test::TestBody()()
250       // susceptible to integer overflows which could make the checks ineffective.
251       // This attempts to verify that we've fixed that. Unfortunately, decrementing
252       // a null pointer is undefined behavior according to the C++ language spec.,
253       // but this should catch the problem on at least some compilers, if not all of
254       // them.
255       const uint8_t* der = nullptr;
>>>     CID 1453375:  Null pointer dereferences  (FORWARD_NULL)
>>>     Incrementing null pointer "der".
256       --der;
257       Input buf;
258       ASSERT_EQ(Success, buf.Init(der, 0));
259       Reader input(buf);
260       uint16_t b;
261       ASSERT_EQ(Result::ERROR_BAD_DER, input.Read(b));

I wonder whether a round trip via uintptr_t would help this.

uintptr_t derint = reinterpret_cast<uintptr_t>(nullptr) - 1;
auto der = reinterpret_cast<const uint8_t*>(derint);
// The spec allows for uintptr_t to be bigger than a pointer, 
// so best to be sure that we're getting underflow.
ASSERT_EQ(sizeof(der), sizeof(derint));

If that doesn't work out, then we might want to just suppress warnings.

Comment 1

[Martin Thomson [:mt:]]

Attachment: Bug 1578626 - Remove undefined nullptr decrement, r=keeler

This uses uintptr_t to avoid the worst. It still looks terrible and might trip
static analysis warnings, but the reinterpret_cast should hide that.

This assumes that sizeof(uintptr_t) == sizeof(void*), so I've added an assertion
so that we'll at least fail the test on those systems. (We could use
GTEST_SKIP instead, but we don't have that in the version of gtest that we use.)

Comment 2

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/d2485b1c997ec9791428290e63d3c737094599a9