search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.36k stars 12.14k forks source link

insufficient evaluation of assumptions for null pointer checks #47964

Open KristofSzabados opened 3 years ago

KristofSzabados commented 3 years ago
Bugzilla Link 48620
Version unspecified
OS Windows NT
CC @devincoughlin

Extended Description

First of all please note that this bug was originally found while using CodeChecker, their customer support person directed me here, claiming the issue is actually coming from clan tidy or static analyzer.

The original bugreport: https://github.com/Ericsson/codechecker/issues/3098 (although it is closed it looks to be watched, so if possible I would propose to have the communication there to have a single channel for a single issue)

The content of the original bugreport: Describe the bug Many of the null pointer checks reported for Titan.core is similar to this particular one. The steps reported by codechecker: " 1, PreGenRecordOf.cc:38242:8: Calling 'PREGENSETOFHEXSTRING::replace' 2, PreGenRecordOf.cc:36817:1: Entered call from 'PREGENSETOFHEXSTRING_template::replace' 3, PreGenRecordOf.cc:36820:5: Assuming field 'val_ptr' is not equal to NULL 4, PreGenRecordOf.cc:36823:1: Calling 'PREGENSETOF__HEXSTRING::set_size' 5, PreGenRecordOf.cc:36848:1: Entered call from 'PREGENSETOFHEXSTRING::replace' 6, PreGenRecordOf.cc:36850:5: Assuming 'new_size' is >= 0 7, PreGenRecordOf.cc:36855:1: Null pointer value stored to field 'value_elements' 8, PreGenRecordOf.cc:36869:5: Assuming 'new_size' is <= field 'n_elements' 9, PreGenRecordOf.cc:36823:1: Returning from 'PREGENSETOFHEXSTRING::set_size' 10, PreGenRecordOf.cc:36824:17: Assuming 'i' is < 'index' 11, PreGenRecordOf.cc:36824:17: Entering loop body "

The interesting parts of the code: //with line numbers added as comment to clarify the situation. part of "replace" " PREGENSETOFHEXSTRING PREGENSETOFHEXSTRING::replace(int index, int len, const PREGENSETOF__HEXSTRING& repl) const //line 36817 { if (val_ptr == NULL) TTCN_error("The first argument of replace() is an unbound value of type @​PreGenRecordOf.PREGEN_SET_OF_HEXSTRING."); if (repl.val_ptr == NULL) TTCN_error("The fourth argument of replace() is an unbound value of type @​PreGenRecordOf.PREGEN_SET_OF_HEXSTRING."); check_replace_arguments(val_ptr->n_elements, index, len, "@PreGenRecordOf.PREGEN_SET_OF_HEXSTRING","element"); PREGENSETOF__HEXSTRING ret_val; ret_val.set_size(val_ptr->n_elements + repl.val_ptr->n_elements - len); //line 36823 for (int i = 0; i < index; i++) { // line 36824 "

part of set_size: " void PREGENSETOF__HEXSTRING::set_size(int new_size) // line 36848 { if (new_size < 0) TTCN_error("Internal error: Setting a negative size for a value of type @​PreGenRecordOf.PREGEN_SET_OF_HEXSTRING."); if (val_ptr == NULL) { val_ptr = new recordof_setof_struct; val_ptr->ref_count = 1; val_ptr->n_elements = 0; val_ptr->value_elements = NULL; } else if (val_ptr->ref_count > 1) { struct recordof_setof_struct new_val_ptr = new recordof_setof_struct; new_val_ptr->ref_count = 1; new_val_ptr->n_elements = (new_size < val_ptr->n_elements) ? new_size : val_ptr->n_elements; new_val_ptr->value_elements = (HEXSTRING)allocate_pointers(new_val_ptr->n_elements); for (int elem_count = 0; elem_count < new_val_ptr->n_elements; elem_count++) { if (val_ptr->value_elements[elem_count] != NULL){ new_val_ptr->value_elements[elem_count] = new HEXSTRING((val_ptr->value_elements[elem_count])); } } clean_up(); val_ptr = new_val_ptr; } if (new_size > val_ptr->n_elements) { "

some minor background context on what the code is meant to do here: The replace function is replacing the elements in the current "list", len elements from index, with the elements of repl.

The path through set_size indicates that:

we are creating a new object to store the result, with initially 0 elements.
"'new_size' is >= 0" and "'new_size' is <= field 'n_elements'" can only happen if new_size == 0, since val_ptr->n_elements is set to 0 in line 36854

In the replace function (calling site of set_size) this would mean, that set_size is called with 0 as the actual value of the parameter. that is "val_ptr->n_elements + repl.val_ptr->n_elements - len" == 0. ( please note that this means that all elements of the initial list is replaced with a list of 0 elements ) This is a valid scenario so far. This however also means, that the call to the function "check_replace_arguments" in replace will only succeed if idx + len <= value_length is true in its body (should we try to replace more elements then what is present will result in an error). This means that the value of "index" in "replace" has to be 0.

At which point the assumption of "'i' is < 'index'" is invalid and the loop is not entered ... never reaching the site that could result in dereferencing a null pointer.

CodeChecker version 6.15

To Reproduce run codechecker on our project.

Expected behaviour This is quite a complex situation ... but maybe the analyzer used in codechecker could be improved to recognize such situations too ... and not report an error for them.

Desktop (please complete the following information)

OS: Win10
Browser: Firefox
Version: 83.0

Additional context the project used is titan.core: https://github.com/eclipse/titan.core The file in question is generated during build, not under version control.

KristofSzabados commented 3 years ago

assigned to @devincoughlin