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 2 commits into
Conversation
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?
There was a problem hiding this comment.
In my opinion, it is beneficial to keep the local prototypes at the top for HDF4. It's easier to get a quick overview of all the local functions in a file. That is convenient when reusing is being considered.
I'm working my way down the list... Thank you, all! |
|
@rouault I think offset should also be verified to be >= 0, like length, in case of corrupted files. |
|
Hi @rouault, we're going to have code freeze this Friday, Aug 21. Would you be able to finish this PR for review so it can be merged by then? FYI, this release is planned to be the last release of HDF4. Thanks so much! |
done |
Thank you! |
|
Hi @rouault, I forgot to ask but, yes, please provide the reproducer when you get a chance. I go ahead and approve it now to speed things up. |
unfortunately I seem to have cleaned up the ossfuzz output directory where it had been generated |
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