Fix potential double free issue in FileInfoUtils_FillFileInfoWithNewestFilesInDir #628
Conversation
…function where, if an error occurs after the FileInfo structure has been partially populated, the cleanup code incorrectly handles the fileNames fields. Specifically, the cleanup code frees the fileNames fields but does not properly reset these fields in the input array.
Nox-MSFT
changed the title
| @@ -168,13 +168,11 @@ bool FileInfoUtils_FillFileInfoWithNewestFilesInDir(FileInfo* logFiles, size_t l | |||
|
|
|||
| if (!succeeded) | |||
| { | |||
| // Free all the file names, if allocated. | |||
There was a problem hiding this comment.
it would be better to use a temporary FileInfo array and not write out to logFiles side-effect until we know that all of it succeeded in the temporary array. That way, at the end, it can unconditionally always free the temporary array.
| @@ -168,13 +168,11 @@ bool FileInfoUtils_FillFileInfoWithNewestFilesInDir(FileInfo* logFiles, size_t l | |||
|
|
|||
| if (!succeeded) | |||
| { | |||
| // Free all the file names, if allocated. | |||
There was a problem hiding this comment.
L146: should we be doing a stat() before checking type? I imagine that stat touches the file, while the S_ just are checking attribute bits.
| @@ -168,13 +168,11 @@ bool FileInfoUtils_FillFileInfoWithNewestFilesInDir(FileInfo* logFiles, size_t l | |||
|
|
|||
| if (!succeeded) | |||
| { | |||
| // Free all the file names, if allocated. | |||
There was a problem hiding this comment.
nit/future: FileInfoUtils_InsertFileInfoIntoArray sorts each time it's called. Probably better to add to current position in array and then sort at end.
| @@ -168,13 +168,11 @@ bool FileInfoUtils_FillFileInfoWithNewestFilesInDir(FileInfo* logFiles, size_t l | |||
|
|
|||
| if (!succeeded) | |||
| { | |||
| // Free all the file names, if allocated. | |||
There was a problem hiding this comment.
L158: there's MAX_FILES_TO_SCAN and there's logFileSize. What happens MAX_FILES_TO_SCAN is greater (or much greater!) than logFileSize?
|
|
||
| memset(&logFileEntry, 0, sizeof(FileInfo)); | ||
| free(logFiles[i].fileName); | ||
| memset(&logFiles[i], 0, sizeof(FileInfo)); |
There was a problem hiding this comment.
nit: no need to memset on each iteration. After this loop, just call memset like done on L107.
| @@ -168,13 +168,11 @@ bool FileInfoUtils_FillFileInfoWithNewestFilesInDir(FileInfo* logFiles, size_t l | |||
|
|
|||
| if (!succeeded) | |||
| { | |||
| // Free all the file names, if allocated. | |||
| for (int i = 0; i < logFileSize; ++i) | |||
There was a problem hiding this comment.
note: technically speaking, a caller shouldn't do anything with out parameters on failure.
int *p
if (!foo(&p)) {
// shouldn't reference p!!!
}So L219's "done" really shouldn't do anything if the call on L219 failed.
Fix an issue in the
FileInfoUtils_FillFileInfoWithNewestFilesInDir()function where, if an error occurs after theFileInfostructure has been partially populated, the cleanup code incorrectly handles thefileNamefields. Specifically, the cleanup code frees the fileName fields but does not properly reset these fields in the input array.