Skip to content
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

JPEG: Handle errors when saving with libjpeg #431

Merged
merged 2 commits into from
Mar 11, 2024
Merged

Conversation

smcv
Copy link
Contributor

@smcv smcv commented Mar 11, 2024

  • JPEG: Factor out the middle of IMG_SaveJPG_RW_jpeglib

    No functional change intended. We'll need to use setjmp() in this
    function in a subsequent commit, so ensure that its state doesn't
    include any local variables that are used both "above" and "below"
    the stack level at which we will call setjmp().

  • JPEG: Add error-recovery when saving with libjpeg

    Because we have set up libjpeg to use my_error_exit, we need to call
    setjmp() before the first time it might possibly call longjmp().
    Otherwise, on error we'll do a non-local goto to an uninitialized
    pointer and crash.


SDL3 version of #432. Untested (there's no SDL3 in the scout SDK yet) but the only difference is the name of a constant.

@smcv
Copy link
Contributor Author

smcv commented Mar 11, 2024

The Windows CI build says:

D:/a/SDL_image/SDL_image/src/IMG_jpg.c:489:18: error: variable 'jpeg_surface' might be clobbered by 'longjmp' or 'vfork' [-Werror=clobbered]

@smcv smcv marked this pull request as draft March 11, 2024 17:49
@sezero
Copy link
Contributor

sezero commented Mar 11, 2024

Maybe we should separate longjmp handling and the rest like we did for libpng code in 3e42a4f (followed by minor simplification 135bec7) (also applied to SDL2 branch)

@smcv
Copy link
Contributor Author

smcv commented Mar 11, 2024

Maybe we should separate longjmp handling and the rest like we did for libpng code in 3e42a4f (followed by minor simplification 135bec7) (also applied to SDL2 branch)

Yes, I think that's going to be necessary. I'm trying to work out how to do that in the most reviewable way.

No functional change intended. We'll need to use setjmp() in this
function in a subsequent commit, so ensure that its state doesn't
include any local variables that are used both "above" and "below"
the stack level at which we will call setjmp().

Signed-off-by: Simon McVittie <[email protected]>
Because we have set up libjpeg to use my_error_exit, we need to call
setjmp() before the first time it might possibly call longjmp().
Otherwise, on error we'll do a non-local goto to an uninitialized
pointer and crash.

Resolves: libsdl-org#429
Signed-off-by: Simon McVittie <[email protected]>
@smcv smcv marked this pull request as ready for review March 11, 2024 19:50
@smcv
Copy link
Contributor Author

smcv commented Mar 11, 2024

OK, how's this?

@sezero
Copy link
Contributor

sezero commented Mar 11, 2024

Looks good, I think. We should do the same to the load function too.

@smcv
Copy link
Contributor Author

smcv commented Mar 11, 2024

We should do the same to the load function too.

Ideally yes, but I'd prefer that sort of refactoring not to be a blocker for a crash fix.

@sezero
Copy link
Contributor

sezero commented Mar 11, 2024

@slouken ?

@slouken
Copy link
Collaborator

slouken commented Mar 11, 2024

Looks good!

@slouken slouken merged commit 5baa3c8 into libsdl-org:main Mar 11, 2024
5 checks passed
@sezero
Copy link
Contributor

sezero commented Mar 11, 2024

We should adapt to SDL2 too and do the same for loader function as well, later.

@smcv
Copy link
Contributor Author

smcv commented Mar 12, 2024

We should adapt to SDL2 too

That was #432 (which is actually the version I wrote first)

and do the same for loader function as well, later

#435

@smcv smcv deleted the issue429 branch March 13, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants