freedroid: Segfault after intro

[smcv]

Prerequisites:

• Debian testing (Debian 12 alpha)
• Video: GNOME 43 in Wayland mode (with Mesa 22.2.0 on AMD Vega, if it matters)
• Audio: Pipewire 0.3.59, with pipewire-pulse emulating PulseAudio
• apt install freedroid (Debian package version 1.0.2+cvs040112-7) (not to be confused with 3D freedroidrpg, which is different)
• Some relevant libraries:
  • libsdl1.2-compat either 1.2.58-1 (packaged in Debian experimental) or commit 63e4393 (locally built)
  • libsdl2-2.0-0 version 2.24.1+dfsg-1

To reproduce:

• freedroid
• LD_LIBRARY_PATH=.../sdl12-compat/_build freedroid
• SDL_VIDEODRIVER=wayland LD_LIBRARY_PATH=.../sdl12-compat/_build freedroid

Each time, click once to pass the Briefing screen, then a second time to pass the description of your robot.

Expected result: all work

Actual result: with sdl12-compat, it crashes.

Video info summary from SDL:
----------------------------------------------------------------------
Is it possible to create hardware surfaces: no
Is there a window manager available: yes
Are hardware to hardware blits accelerated: no
Are hardware to hardware colorkey blits accelerated: no
Are hardware to hardware alpha blits accelerated: no
Are software to hardware blits accelerated: no
Are software to hardware colorkey blits accelerated: no
Are software to hardware alpha blits accelerated: no
Are color fills accelerated: no
Total amount of video memory in Kilobytes: 262144
Pixel format of the video device: bpp = 32, bytes/pixel = 4
Video Driver Name: wayland
----------------------------------------------------------------------
Found new graphics-theme: classic 
Found new graphics-theme: lanzz 
Found new graphics-theme: para90 
Game starts using theme: classic
Segmentation fault (core dumped)

#0  SDL_ListRemove (head=0x55b900e608a8, ent=ent@entry=0x55b9a5f4dce0) at ./src/SDL_list.c:70
#1  0x00007f49e83801eb in SDL_InvalidateMap (map=0x55b9a5f4dce0) at ./src/video/SDL_pixels.c:1052
#2  SDL_MapSurface (src=src@entry=0x55b9a5f4d7c0, dst=dst@entry=0x55b9a5e60870) at ./src/video/SDL_pixels.c:1075
#3  0x00007f49e83847ca in SDL_LowerBlit_REAL (src=0x55b9a5f4d7c0, srcrect=0x7ffcaf3e5d80, dst=0x55b9a5e60870, dstrect=0x7ffcaf3e5e00)
    at ./src/video/SDL_surface.c:701
#4  0x00007f49e8384a59 in SDL_UpperBlit_REAL (src=0x55b9a5f4d7c0, srcrect=<optimized out>, dst=0x55b9a5e60870, dstrect=0x7ffcaf3e5e00)
    at ./src/video/SDL_surface.c:807
#5  0x00007f49e8aef62c in SDL_UpperBlit
    (src12=0x55b9a5f4dd70, srcrect12=<optimized out>, dst12=0x55b9a5e60970, dstrect12=0x7ffcaf3e5e48)
    at /home/desktop/tmp/sdl12-compat/src/SDL12_compat.c:4772
#6  0x000055b9a3b26e07 in PutInfluence (x=x@entry=-1, y=y@entry=-1) at view.c:290
#7  0x000055b9a3b27ee4 in Assemble_Combat_Picture (mask=mask@entry=2) at view.c:228
#8  0x000055b9a3b0c9ea in main (argc=<optimized out>, argv=<optimized out>) at main.c:140

[icculus]

SaveDestAlpha strikes again. This is from valgrind:

==3004919== Conditional jump or move depends on uninitialised value(s)
==3004919==    at 0x5428BDC: SDL_malloc_REAL (SDL_malloc.c:5405)
==3004919==    by 0x539D1C2: SDL_malloc (SDL_dynapi_procs.h:408)
==3004919==    by 0x490B9E6: SaveDestAlpha (SDL12_compat.c:6198)
==3004919==    by 0x490BF1C: SDL_UpperBlit (SDL12_compat.c:6288)
==3004919==    by 0x127CB5: ??? (in /usr/games/freedroid)
==3004919==    by 0x128A3B: ??? (in /usr/games/freedroid)
==3004919==    by 0x10D1D1: main (in /usr/games/freedroid)
==3004919== 
==3004919== Conditional jump or move depends on uninitialised value(s)
==3004919==    at 0x490BB83: SaveDestAlpha (SDL12_compat.c:6217)
==3004919==    by 0x490BF1C: SDL_UpperBlit (SDL12_compat.c:6288)
==3004919==    by 0x127CB5: ??? (in /usr/games/freedroid)
==3004919==    by 0x128A3B: ??? (in /usr/games/freedroid)
==3004919==    by 0x10D1D1: main (in /usr/games/freedroid)
==3004919== 
==3004919== Conditional jump or move depends on uninitialised value(s)
==3004919==    at 0x490BE46: RestoreDestAlpha (SDL12_compat.c:6266)
==3004919==    by 0x490BFB0: SDL_UpperBlit (SDL12_compat.c:6297)
==3004919==    by 0x127CB5: ??? (in /usr/games/freedroid)
==3004919==    by 0x128A3B: ??? (in /usr/games/freedroid)
==3004919==    by 0x10D1D1: main (in /usr/games/freedroid)
==3004919== 
==3004919== Conditional jump or move depends on uninitialised value(s)
==3004919==    at 0x4848842: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==3004919==    by 0x542896E: real_malloc (SDL_malloc.c:5310)
==3004919==    by 0x5428BF5: SDL_malloc_REAL (SDL_malloc.c:5409)
==3004919==    by 0x539D1C2: SDL_malloc (SDL_dynapi_procs.h:408)
==3004919==    by 0x490B9E6: SaveDestAlpha (SDL12_compat.c:6198)
==3004919==    by 0x490BF1C: SDL_UpperBlit (SDL12_compat.c:6288)
==3004919==    by 0x12787B: ??? (in /usr/games/freedroid)
==3004919==    by 0x1289A3: ??? (in /usr/games/freedroid)
==3004919==    by 0x10D1D1: main (in /usr/games/freedroid)
==3004919== 
==3004919== Warning: set address range perms: large range [0x59c87040, 0xc4b3d4bc) (undefined)
==3004919== Conditional jump or move depends on uninitialised value(s)
==3004919==    at 0x490BB83: SaveDestAlpha (SDL12_compat.c:6217)
==3004919==    by 0x490BF1C: SDL_UpperBlit (SDL12_compat.c:6288)
==3004919==    by 0x12787B: ??? (in /usr/games/freedroid)
==3004919==    by 0x1289A3: ??? (in /usr/games/freedroid)
==3004919==    by 0x10D1D1: main (in /usr/games/freedroid)
==3004919== 
==3004919== Conditional jump or move depends on uninitialised value(s)
==3004919==    at 0x490BB6F: SaveDestAlpha (SDL12_compat.c:6218)
==3004919==    by 0x490BF1C: SDL_UpperBlit (SDL12_compat.c:6288)
==3004919==    by 0x12787B: ??? (in /usr/games/freedroid)
==3004919==    by 0x1289A3: ??? (in /usr/games/freedroid)
==3004919==    by 0x10D1D1: main (in /usr/games/freedroid)
==3004919== 
==3004919== Invalid read of size 4
==3004919==    at 0x490BB45: SaveDestAlpha (SDL12_compat.c:6219)
==3004919==    by 0x490BF1C: SDL_UpperBlit (SDL12_compat.c:6288)
==3004919==    by 0x12787B: ??? (in /usr/games/freedroid)
==3004919==    by 0x1289A3: ??? (in /usr/games/freedroid)
==3004919==    by 0x10D1D1: main (in /usr/games/freedroid)
==3004919==  Address 0xa7a63f8 is 0 bytes after a block of size 16,424 alloc'd
==3004919==    at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==3004919==    by 0x542896E: real_malloc (SDL_malloc.c:5310)
==3004919==    by 0x5428BF5: SDL_malloc_REAL (SDL_malloc.c:5409)
==3004919==    by 0x5390068: SDL_SIMDAlloc_REAL (SDL_cpuinfo.c:1125)
==3004919==    by 0x549B833: SDL_CreateRGBSurfaceWithFormat_REAL (SDL_surface.c:152)
==3004919==    by 0x549B97A: SDL_CreateRGBSurface_REAL (SDL_surface.c:198)
==3004919==    by 0x549DC81: SDL_ConvertSurface_REAL (SDL_surface.c:1196)
==3004919==    by 0x539E1D7: SDL_ConvertSurface (SDL_dynapi_procs.h:495)
==3004919==    by 0x490C480: SDL_ConvertSurface (SDL12_compat.c:6404)
==3004919==    by 0x490C623: SDL_DisplayFormatAlpha (SDL12_compat.c:6450)
==3004919==    by 0x11634D: ??? (in /usr/games/freedroid)
==3004919==    by 0x11AD3F: ??? (in /usr/games/freedroid)
==3004919== 
==3004919== 
==3004919== More than 10000000 total errors detected.  I'm not reporting any more.
==3004919== Final error counts will be inaccurate.  Go fix your program!
==3004919== Rerun with --error-limit=no to disable this cutoff.  Note
==3004919== that errors may occur in your program without prior warning from
==3004919== Valgrind, because errors are no longer being displayed.

[sezero]

This shouldn't be fixed by 35488c6, it should be #248 instead.

[icculus]

Ah, yes, my bad.

[icculus]

ok, the valgrind issues are resolved on sdl12-compat's end (but note that freedroid doesn't initialize the destination rectangle correctly in any case, it doesn't initialize the stack-allocated variable before doing dst.x += whatever;), but we're still rendering incorrectly, so this bug is still open.

[icculus]

This is trying to blit from a surface that has its flags set to SDL_RLEACCELOK in SDL-1.2, where sdl12-compat has it set to zero; this is probably okay, it was an internal flag we probably don't use.

But it's also trying to blit to a surface that has its flags set to SDL_SRCALPHA in SDL-1.2...also zero in sdl12-compat.

It creates this destination surface with SDL_DisplayFormatAlpha(), and while the SDL_Surface::format details match, my guess is this flag should be set somewhere along the way, and will fix this issue. I need to dig a little further.

[icculus]

We appear to be checking for this flag in sdl12-compat's SDL_DisplayFormatAlpha, so the surface-to-be-converted doesn't have it for some reason.

[icculus]

Ok, we set the SDL_SRCALPHA flag the same way 1.2 does now, I think, and this partially fixes the droid rendering, as they now aren't backed by a black square but correctly blend with the background.

However: the droid's ID number doesn't render...this appears to be further drama with SaveDestAlpha in sdl12-compat; if I disable alpha saving/restoring, the droid rendering is 100% correct. Don't know what's up there, yet.

Also, shots from your weapon and burn marks where destroyed droids lay are still rendered with black backgrounds, regardless of any of these changes, so there's still some more to do here.

[icculus]

As far as the droid numbers go: the surfaces are flagged SDL_RLEACCEL when run with SDL-1.2, and not with sdl12-compat...but more notably, SDL 1.2 doesn't appear to preserve destination alpha for RLE-encoded surfaces, but only in the run-lengths. The blit lands in here:

https://github.com/libsdl-org/SDL-1.2/blob/main/src/video/SDL_RLEaccel.c#L1194-L1259

And either memcpy's pixels for run-lengths:

https://github.com/libsdl-org/SDL-1.2/blob/main/src/video/SDL_RLEaccel.c#L111-L118

Or blends them with this, which does appear to preserve dest alpha:

https://github.com/libsdl-org/SDL-1.2/blob/main/src/video/SDL_RLEaccel.c#L955-L967

This is getting messy now.

[icculus]

Although, just turning off sdl12-compat's efforts to preserve destination alpha fixes all the rendering issues completely, so I'm just going to add a quirk and leave it at that for now.

[icculus]

(I must have had something else broken, it's only the droid numbers that need the fix I just pushed. Everything else is rendering correctly with or without this patch.)