View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0003550OpenFOAMPatchpublic2020-09-18 13:292020-09-18 14:54
ReporterwyldckatAssigned Tohenry 
PrioritylowSeverityminorReproducibilitysometimes
Status resolvedResolutionfixed 
Product Versiondev 
Fixed in Versiondev 
Summary0003550: 'malloc' in 'wmkdep.l' assumes memory preset to zero, only one situation where it can cause issues
DescriptionIn 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.
TagsNo tags attached.

Activities

wyldckat

2020-09-18 13:29

updater  

patch_wmkdep_v1.patch (424 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);
     }
patch_wmkdep_v1.patch (424 bytes)
wmkdep.l (12,169 bytes)

henry

2020-09-18 13:36

manager   ~0011515

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?

wyldckat

2020-09-18 13:42

updater   ~0011516

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...

henry

2020-09-18 13:56

manager   ~0011517

sourcePath[basePos - sourceFile] = '\0';

would be even more explicit, see

commit 067ab99f0fc238adca948b39687e4301b57c10d4

wyldckat

2020-09-18 14:54

updater   ~0011518

@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.

Issue History

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