Fix potential READ memory access#26782
Conversation
|
@asmorkalov, I see that tests are failing due to old GitHub dependencies. We probably want a dependabot config. This is what we have for AVIF: https://github.com/AOMediaCodec/libavif/blob/main/.github/dependabot.yml It will make PRs once a month to update your CI files (if needed) |
|
@vrabaud Can you elaborate on this a bit more? IMHO there is nothing wrong with the original code. |
|
@vrabaud Could you bring more details. I do not see any difference. |
|
My code was wrong, the new one works. Temporary variables do not play well with |
|
It's still a bit inconsistent to me. I'm aware that setjmp/longjmp has the potential to bypass destructors (and thereby causing memory leaks) in certain situations. If this is a problem in 'processing_start' (not sure if it is), wouldn't 'readHeader' suffer from the same issue? So I think either local 'PngPtrs' should be avoided in both cases or in none of them. Also, if we go the route of working directly with 'm_png_ptrs', we should probably clear it in case of failures. Like it is done in 'processing_finish'. |
|
There is another small thing I find unfortunate: The default constructor of 'PngPtrs' allocates data. That means, when a PngDecoder is instantiated, we allocate stuff that gets thrown away on the very first usage of this decoder instance. I guess it's better to add an explicit 'init' method to 'PngPtrs' and let the default constructor just zero-initialize all member variables. |
786603f to
7a9f2cd
Compare
|
@warped-rudi , I indeed refactored to be more consistent. The READ memory access is fixed by the extra |
3c6af8f to
d7be006
Compare
|
I added a fix for https://oss-fuzz.com/testcase-detail/5048650127966208 |
|
That look good to me! Two more nit-picks: PngDecoder (as well as PngEncoder) don't have to have protected members. Make them private. Also the question wether or not to call ClearPngPtr() every time setjmp() returns a non-zero value persists. Currently it's only done in processing_finish(). |
|
Linux builds report: |
|
I fixed the two comments.
ClearPngPtr does not need to be called every time setjmp returns a non-zero as it deals with variables outside of the longjmp. Those variables will be cleared normally by the destructor. I removed it from processing_finish. |
Fix potential READ memory access opencv#26782 This fixes https://oss-fuzz.com/testcase-detail/4923671881252864 and https://oss-fuzz.com/testcase-detail/5048650127966208 ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [x] The PR is proposed to the proper branch - [x] There is a reference to the original bug report and related work - [ ] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [ ] The feature is well documented and sample code can be built with the project CMake
This fixes https://oss-fuzz.com/testcase-detail/4923671881252864 and https://oss-fuzz.com/testcase-detail/5048650127966208
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.