covscan: fix miscellaneaous errors #2003
Conversation
jeromemarchand
requested review from
4ast,
drzaeus77 and
yonghong-song
as code owners
src/cc/bcc_elf.c
Outdated
| @@ -373,6 +373,9 @@ static int verify_checksum(const char *file, unsigned int crc) { | |||
| void *buf; | |||
| unsigned int actual; | |||
|
|
|||
| if (file == NULL) | |||
There was a problem hiding this comment.
This one is actually not needed.
DONE:
free(bindir);
if (res && check_crc && !verify_checksum(res, crc))
return NULL;
return res;
The file is guaranteed to be non NULL.
There was a problem hiding this comment.
Good point! Commit b84714a probably made the coverity warning disappeared already.
src/cc/bcc_proc.c
Outdated
| @@ -99,7 +99,7 @@ int bcc_procutils_each_module(int pid, bcc_procutils_modulecb callback, | |||
| while (true) { | |||
| buf[0] = '\0'; | |||
| // From fs/proc/task_mmu.c:show_map_vma | |||
| if (fscanf(procmap, "%lx-%lx %s %llx %s %lu%[^\n]", &begin, &end, perm, | |||
| if (fscanf(procmap, "%lx-%lx %4s %llx %7s %lu%[^\n]", &begin, &end, perm, | |||
There was a problem hiding this comment.
Should %7s to %5s since it MAJOR:MINOR format where MAJOR/MINOR are two byte width?
There was a problem hiding this comment.
It's 7 because the buffer dev is of size 8. But a 6 bytes buffer would be enough. We don't do anything with it after the call to fscanf() anyway.
There was a problem hiding this comment.
Then "7" is okay. thanks for explanation.
There was a problem hiding this comment.
Just want to be sure, %Ns means reading at most N characters, right? Dev number could definitely be shorter.
| @@ -435,8 +438,10 @@ static char *find_debug_via_debuglink(Elf *e, const char *binpath, | |||
|
|
|||
| DONE: | |||
| free(bindir); | |||
| if (res && check_crc && !verify_checksum(res, crc)) | |||
| if (res && check_crc && !verify_checksum(res, crc)) { | |||
There was a problem hiding this comment.
Huh, I just looked at Man page for dirname and it says
These functions may return pointers to statically allocated memory which may be overwritten by subsequent calls. Alternatively, they may return a pointer to some part of path, so that the string referred to by path should not be modified or freed until the pointer returned by the function is no longer required.
So maybe the free(bindir) part is also not really correct per say?
There was a problem hiding this comment.
I just checked glibc code, For the path without '/' (e.g., a.out), a static memory inside dirname is returned.
It looks the best way is to declare an array, e.g., bindir[PATH_MAX] as the input to the bindir function.
This code will not execute in typical case. It will be executed if a separate debug file is specified, which is probably we won't hit the issue often.
There was a problem hiding this comment.
Good point! The result of strdup(binpath) should be preserved in a tmp pointer and that pointer freed instead of bindir.
src/cc/bcc_proc.c
Outdated
| @@ -99,7 +99,7 @@ int bcc_procutils_each_module(int pid, bcc_procutils_modulecb callback, | |||
| while (true) { | |||
| buf[0] = '\0'; | |||
| // From fs/proc/task_mmu.c:show_map_vma | |||
| if (fscanf(procmap, "%lx-%lx %s %llx %s %lu%[^\n]", &begin, &end, perm, | |||
| if (fscanf(procmap, "%lx-%lx %4s %llx %7s %lu%[^\n]", &begin, &end, perm, | |||
There was a problem hiding this comment.
Just want to be sure, %Ns means reading at most N characters, right? Dev number could definitely be shorter.
|
@jeromemarchand any update on this one? |
dirname() may return pointers to statically allocated memory. Don't free the pointer it returns.
jeromemarchand
force-pushed
the
covscan2
branch
from
db66076 to
3d9c2cb
Compare
|
Did that last update addressed all of your concerns? I dropped the unneeded patch, reduced the dev buffer to 6 bytes and fixed the free()/dirname() issue. |
|
Sorry, I did not notice you made the change. The change actually looks good. Will test it now. |
|
[buildbot, test this please] |
2 similar comments
|
[buildbot, test this please] |
|
[buildbot, test this please] |
* Coverity #def53: COPY_PASTE_ERROR * Coverity #def18: DC.STREAM_BUFFER. Double-check max length of dev * Coverity #def44: MISSING_BREAK. This looks like it should be here * Coverity #def67: STRING_NULL: potential OOB read if 0 bytes read. * Coverity #def66: FORWARD_NULL: potential null ptr deref * Coverity #def17: RESOURCE_LEAK: missing free() * Dont free the result of dirname dirname() may return pointers to statically allocated memory. Don't free the pointer it returns.
This fixes a few errors discovered by a coverity scan.