Fix buffer overflow, resource leaks, missing error checks and possible use of uninitialized variables#563
Fix buffer overflow, resource leaks, missing error checks and possible use of uninitialized variables#563aaronmerey wants to merge 10 commits intoicl-utk-edu:masterfrom
Conversation
papi_load_derived_events and papi_load_derived_events_component might pass string literal "papi_events.csv" to open_event_table using the PAPI_EVENT_FILE alias. In this case, open_event_table may attempt to call sprintf where PAPI_EVENT_FILE alias is both the destination buffer and the source string for the format argument. This is undefined behaviour in two ways: (1) The destination buffer for sprintf cannot also be used as a source for a format argument. (2) String literals are read-only and should not be modified. Fix this by copying PAPI_EVENT_FILE to a modifiable local buffer for use in papi_load_derived_events and papi_load_derived_events_component.
In exp_container_insert_element, variable chunk is set to the number of chunks already allocated in exp_container. chunk is set to a number between 0 and EXP_CONTAINER_ENTRIES - 1. If used_entries == total_entries, chunk is incremented by 1 and used as an index into exp_container->ptr_array. This array has size EXP_CONTAINER_ENTRIES. This results in an overrun if chunk is equal to EXP_CONTAINER_ENTRIES after the increment. Fix this by returning SDE_EINVAL if chunk >= EXP_CONTAINER_ENTRIES.
Check pread return value for -1 and handle error case.
Fix error cases in generateEventList that return early without calling closedir on DIR* variables dir and d.
Ensure variables allNames, allPMID and allFetch are freed for all early returns for error cases.
Ensure localname is freed before early return for error case.
Ensure malloc, calloc and strdup return values are checked for errors and free all allocated resources when an error is encountered.
Ensure variable evt_name_copy is freed if returning early due to error.
Initialize variables with a default value to avoid warnings regarding possible uninitialized use in static analyzer tools such as coverity.
|
@aaronmerey Thank you for submitting this PR. I will take a look through it. |
| if (event_fd == -1) { break; } | ||
|
|
||
| int sz = pread(event_fd, read_buff, PAPI_MAX_STR_LEN, 0); | ||
| if (sz == -1) { HANDLE_STRING_ERROR; } |
There was a problem hiding this comment.
The formatting here for HANDLE_STRING_ERROR is different from the rest below. I did not an counter any error if the formatting here mimics the formatting below. Can you change the formatting here to be like below or vice versa?
| } | ||
|
|
||
| if (insert_in_list(name,units,description,filename)!=PAPI_OK) { | ||
| err = PAPI_ECMP; |
There was a problem hiding this comment.
insert_in_list can also return PAPI_ENOMEM. Can you change this check such that you set err equal to insert_in_list and then if a failure does occur you can jump to done_error?
| } | ||
|
|
||
| if (insert_in_list(name,units,description,filename)!=PAPI_OK) { | ||
| err = PAPI_ECMP; |
| } | ||
|
|
||
| if (insert_in_list(name,units,description,filename)!=PAPI_OK) { | ||
| err = PAPI_ECMP; |
| fn_exit: | ||
| free(allNames); // Locals allocations not needed anymore. | ||
| free(allPMID); // .. the pmIDs we read. | ||
| pcp_pmFreeResult(allFetch); // .. release the results we fetched. |
There was a problem hiding this comment.
From documentation:
As a general rule, if the called function returns an error status, then no allocation is done, the pointer to the variable sized result is undefined, and free, pmFreeLabelSets, or pmFreeResult should not be called.
Therefore, before calling pcp_pmFreeResult we need to check:
if (all_fetch != NULL) {
pcp_pmFreeResult(allFetch);
}
|
|
||
| for (k = 0; k < prstPtr->num_quals; k++){ | ||
| providedQuals[k] = (char*)malloc(sizeof(char)*(PAPI_MAX_STR_LEN+1)); | ||
| PAPIERROR("Could not allocate %lu bytes of memory for providedQuals element.", |
There was a problem hiding this comment.
The allocation for providedQuals[k] needs to be checked if it is equal to NULL and then you subsequently have the PAPIERROR, ret = PAPI_ENOMEM, and goto done.
| for (k = 0; k < numProvidedQuals; k++) { | ||
| wholeQual1 = strdup(providedQuals[k]); | ||
| if (wholeQual1 == NULL) { | ||
| PAPIERROR("Failed to make copy of qualifier %s", providedQuals[k]); |
There was a problem hiding this comment.
Can you make this PAPIERROR message more descriptive? Same as the two that follow for the qualifiers.
2 participants
Pull Request Description
Patch series to fix buffer overflow, resource leaks, missing error checks and possible use of uninitialized variables