hfiledd.c: fix integer overflow and memleak on corrupted file#874
hfiledd.c: fix integer overflow and memleak on corrupted file#874rouault wants to merge 1 commit into
Conversation
rouault
requested review from
bmribler,
brtnfld,
byrnHDF,
derobins,
fortnern,
glennsong09,
jhendersonHDF,
lrknox,
mattjala,
schwehr and
vchoi-hdfgroup
as code owners
schwehr
left a comment
schwehr
left a comment
There was a problem hiding this comment.
The change as it is currently written is acceptable for me. My comments are mostly just nits that probably can't go in as they deviate from the established style in hdf4.
@bmribler Can you respond on the style issues here for @rouault ?
I agree with Even that there are likely a lot more issues lurking in this code. A quick AI analysis of the code in just this file found 8 likely places for integer overflows. :(
| #include "hfile_priv.h" | ||
|
|
||
| /* Private routines */ | ||
| static void HTPmemory_cleanup(filerec_t *file_rec); |
There was a problem hiding this comment.
I know this file has traditionally had prototypes and define the code below, but can we switch to just having the function here?
| { | ||
| ddblock_t *bl, *next; /* current ddblock and next ddblock pointers. | ||
| for freeing ddblock linked list */ | ||
| for (bl = file_rec->ddhead; bl != NULL; bl = next) { |
There was a problem hiding this comment.
nit: Can we use C99 and just make it:
for (ddblock_t *bl = file_rec->ddhead, *next; bl != NULL; bl = next) {| static void | ||
| HTPmemory_cleanup(filerec_t *file_rec) | ||
| { | ||
| ddblock_t *bl, *next; /* current ddblock and next ddblock pointers. |
There was a problem hiding this comment.
// Free the ddblock linked list.| } | ||
| file_rec->ddhead = (ddblock_t *)NULL; | ||
|
|
||
| /* Chuck the tag info tree too */ |
There was a problem hiding this comment.
// Deallocate the tag info tree.
I'm working my way down the list... Thank you, all! |
3 participants
Found by running locally oss-fuzz on GDAL with HDF4 support enabled. This is likely just the tip of the iceberg. I fixed 2 issues (an integer overflow and a memory leak), and then retried again the fuzzing, and it immediately found another memory leak...
I can provide the reproducer if needed