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

Allocate video surface object statically as a global #306

Merged
merged 4 commits into from
Aug 26, 2023

Conversation

smcv
Copy link
Contributor

@smcv smcv commented Jul 20, 2023

In #305, we have trouble with libsdl-perl making assumptions about the video surface never being freed or reallocated, and it being a no-op to "free" the video surface.

There is nothing that says a SDL 1.2 surface must always wrap the same SDL 2 surface, so we can maybe accommodate those assumptions by having the video surface be a special-cased global pointer that is never freed, and only reallocate its contents?

This seems to work with libsdl-perl's core_video.t test, but I haven't tested it extensively; consider it a proof-of-concept for now.

  • Factor out Surface20to12InPlace

    This is a step towards making it possible to use the same SDL12_Surface
    for the video surface at all times, and only reallocating its underlying
    SDL 2 SDL_Surface.

  • Factor out FreeSurfaceContents, and handle surface20 more defensively

    This is a step towards making it possible to use the same SDL12_Surface
    for the video surface at all times, and only reallocating its underlying
    SDL 2 SDL_Surface.

  • Factor out enough of SDL_CreateRGBSurface to create surfaces in-place

    The part before we create the SDL 1.2 surface (creating the SDL 2.0
    surface) becomes CreateRGBSurface(), and the part after becomes
    Surface12SetMasks().

  • Allocate video surface object statically as a global

    The SDL Perl bindings incorrectly call SDL_FreeSurface() on the result
    of functions that return a "borrowed" pointer to the video surface,
    namely SDL_SetVideoMode() and SDL_GetVideoSurface().
    (See Tries to free previous results of SDL_SetVideoMode(), leading to use-after-free crash PerlGameDev/SDL#305)

    When we would previously have allocated or freed the video surface
    wrapper object, instead allocate or free its contents in-place.

    When checking whether the video surface exists, because we never destroy
    it, we must now also check whether its underlying SDL2 video surface
    exists.

    Resolves: makes SDL_perl regress when it (incorrectly) frees a former video surface #305

@smcv
Copy link
Contributor Author

smcv commented Jul 20, 2023

@slouken, @icculus: I think this is ready for at least a preliminary "is this a good direction?" review, but it isn't sufficiently tested to land it yet.

@slouken
Copy link
Collaborator

slouken commented Jul 20, 2023

This seems like a good direction to me.

This is a step towards making it possible to use the same SDL12_Surface
for the video surface at all times, and only reallocating its underlying
SDL 2 SDL_Surface.

Signed-off-by: Simon McVittie <[email protected]>
This is a step towards making it possible to use the same SDL12_Surface
for the video surface at all times, and only reallocating its underlying
SDL 2 SDL_Surface.

Signed-off-by: Simon McVittie <[email protected]>
The part before we create the SDL 1.2 surface (creating the SDL 2.0
surface) becomes CreateRGBSurface(), and the part after becomes
Surface12SetMasks().

Signed-off-by: Simon McVittie <[email protected]>
The SDL Perl bindings incorrectly call SDL_FreeSurface() on the result
of functions that return a "borrowed" pointer to the video surface,
namely SDL_SetVideoMode() and SDL_GetVideoSurface().
(See PerlGameDev/SDL#305)

When we would previously have allocated or freed the video surface
wrapper object, instead allocate or free its contents in-place.

When checking whether the video surface exists, because we never destroy
it, we must now also check whether its underlying SDL2 video surface
exists.

Resolves: libsdl-org#305
Signed-off-by: Simon McVittie <[email protected]>
@smcv
Copy link
Contributor Author

smcv commented Aug 26, 2023

Rebased on main and fixed a missing NULL check. Briefly tested with:

  • libsdl-perl: frozen-bubble, pangzero
  • non-Perl: amoebax, armagetronad, enemylines7, powermanga

and seems to work as intended.

@smcv smcv marked this pull request as ready for review August 26, 2023 14:39
@smcv
Copy link
Contributor Author

smcv commented Aug 26, 2023

I also contributed a fix for the incorrect free (PerlGameDev/SDL#306) which was applied to Debian's libsdl-perl package, but I haven't seen any sign of it being reviewed upstream. I also accidentally found PerlGameDev/SDL#307 while testing that change, and it hasn't had any response either, so PerlGameDev/SDL might be relatively dead.

@slouken
Copy link
Collaborator

slouken commented Aug 26, 2023

Okay, well let's merge this then.

Thanks!

@slouken slouken merged commit d86a531 into libsdl-org:main Aug 26, 2023
5 checks passed
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.

makes SDL_perl regress when it (incorrectly) frees a former video surface
2 participants