View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0003550 | OpenFOAM | Patch | public | 2020-09-18 13:29 | 2020-09-18 14:54 |
Reporter | wyldckat | Assigned To | henry | ||
Priority | low | Severity | minor | Reproducibility | sometimes |
Status | resolved | Resolution | fixed | ||
Product Version | dev | ||||
Fixed in Version | dev | ||||
Summary | 0003550: 'malloc' in 'wmkdep.l' assumes memory preset to zero, only one situation where it can cause issues | ||||
Description | In the file 'wmake/src/wmkdep.l', these lines of code are risky and can cause issues in some situations: sourcePath = (char*)malloc(basePos - sourceFile + 1); strncpy(sourcePath, sourceFile, basePos - sourceFile); In essence, the risk here is that 'sourcePath' gets a partial copy of the path on the left of the source file string, but does not assign the null character at the end of the copy. Here is an example, namely if: sourceFile = "singlePhaseTransportModel/singlePhaseTransportModel.H"; then the result will be: sourcePath = "singlePhaseTransportModel?"; the '?' refers to the last byte of the 'sourcePath' char array, which is undefined when using 'strncpy', which quoting from here: http://www.cplusplus.com/reference/cstring/strncpy/ No null-character is implicitly appended at the end of destination if source is longer than num. Thus, in this case, destination shall not be considered a null terminated C string (reading it as such would overflow). This is usually not a problem in the majority of situations, at least on Linux, since the majority of situations the memory is already all preset to NULL... however, there are a few instances where this isn't true, even on Linux. In my situation, this was triggered on Windows, which usually initializes RAM with non-NULL values (allegedly to assist in catching memory leaks), which revealed this issue. The proposed solution is to use the attached changes: - 'patch_wmkdep_v1.patch' shows the proposed change, namely to use 'calloc', which does initialize memory to zeros of the new array. - 'wmkdep.l' for replacing 'wmake/src/wmkdep.l' in OpenFOAM-dev (commit c4f98e7835ce, Sep 17 10:51:29 2020) I've tested this on Windows and Linux and it does not have any other issues. All other 'malloc's in this code seem to be being used safely, since they are followed by string copy functions that always copy/include the null termination character. | ||||
Tags | No tags attached. | ||||
|
patch_wmkdep_v1.patch (424 bytes)
wmkdep.l (12,169 bytes)diff --git a/wmake/src/wmkdep.l b/wmake/src/wmkdep.l index 166a219..b159ee2 100644 --- a/wmake/src/wmkdep.l +++ b/wmake/src/wmkdep.l @@ -144,7 +144,7 @@ int main(int argc, char* argv[]) } else { - sourcePath = (char*)malloc(basePos - sourceFile + 1); + sourcePath = (char*)calloc(basePos - sourceFile + 1, sizeof(char*)); strncpy(sourcePath, sourceFile, basePos - sourceFile); } |
|
Using the the fact that calloc initialises the memory is a bit of a hack, wouldn't it be better to set the null character explicitly so that the intent is clear in the code rather than using a side-effect? |
|
That's what I had initially thought of proposing, namely: sourcePath[basePos - sourceFile] = 0; but then this felt more like a hack than using calloc... even though calloc likely ends up being slower. There is also the function 'strncpy_s', which is standard as of C11: https://en.cppreference.com/w/c/string/byte/strncpy - quoting: Although truncation to fit the destination buffer is a security risk and therefore a runtime constraints violation for strncpy_s, it is possible to get the truncating behavior by specifying count equal to the size of the destination array minus one: it will copy the first count bytes and append the null terminator as always: strncpy_s(dst, sizeof dst, src, (sizeof dst)-1); Although its usage is a bit more cumbersome to the eye and wasn't standard years ago... |
|
sourcePath[basePos - sourceFile] = '\0'; would be even more explicit, see commit 067ab99f0fc238adca948b39687e4301b57c10d4 |
|
@Henry: Perfect, I've tested it on Windows and works as intended. Marking this issue as resolved in the commit above. PS: I'm getting rusty... I had forgotten about '\0' being the null character. |
Date Modified | Username | Field | Change |
---|---|---|---|
2020-09-18 13:29 | wyldckat | New Issue | |
2020-09-18 13:29 | wyldckat | File Added: patch_wmkdep_v1.patch | |
2020-09-18 13:29 | wyldckat | File Added: wmkdep.l | |
2020-09-18 13:36 | henry | Note Added: 0011515 | |
2020-09-18 13:42 | wyldckat | Note Added: 0011516 | |
2020-09-18 13:56 | henry | Note Added: 0011517 | |
2020-09-18 14:54 | wyldckat | Assigned To | => henry |
2020-09-18 14:54 | wyldckat | Status | new => resolved |
2020-09-18 14:54 | wyldckat | Resolution | open => fixed |
2020-09-18 14:54 | wyldckat | Fixed in Version | => dev |
2020-09-18 14:54 | wyldckat | Note Added: 0011518 |