CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
Fix potential READ memory access #26782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.