-
Type:
Technical Debt
-
Resolution: Fixed
-
Priority:
Minor - P4
-
Affects Version/s: None
-
Component/s: Lint
-
5
-
StorEng - Defined Pipeline
Summary
This popped up during the implementation of WT-10043. We have a handful of WT_ASSERTs which are checking the results of functions that with side effects such as allocating memory or assigning values to structs.
// This sets ref_ikey WT_ASSERT(session, __wt_atomic_cas_ptr(&ref->ref_ikey, (WT_IKEY *)oldv, ikey)); // This allocates mem for &key WT_ASSERT(session, __wt_row_leaf_key(session, page, rip, &key, false) == 0); // This allocated mem for &ctmp WT_ASSERT(session, __wt_scr_alloc(session, dsk->mem_size, &ctmp)); // This loads data from buf into ctmp WT_ASSERT(session, btree->compressor->decompress(btree->compressor, &session->iface, (uint8_t *)buf->data + WT_BLOCK_COMPRESS_SKIP, buf->size - WT_BLOCK_COMPRESS_SKIP, (uint8_t *)ctmp->data + WT_BLOCK_COMPRESS_SKIP, ctmp->memsize - WT_BLOCK_COMPRESS_SKIP, &result_len) == 0);
The problem with this is that if asserts are disabled, these functions are not run and can potentially break WiredTiger. In the case of the first example it can lead to a null pointer dereference.
Luckily this is currently not possible in the code base as all these examples are wrapped inside an #ifdef HAVE_DIAGNOSTIC which guarantees WT_ASSERTS are run, but for cleanliness we should move these functions outside of their WT_ASSERTs and instead only assert on their returned values.