libaldmb: Crashes with games that are using packfiles

[Rondom]

Both alex4 and kraptor crash at approximately the same place. alex4 crashes right at the start, while kraptor crashes once you start a new game.

I guess, it is related to packfiles/datafiles reading. open-invaders which is another allegro4-game works fine, but it does use separate mod-files, which (I assume) is why it does not crash. Backtrace is from my Debian-system, but it has been reproduced on Arch as well.

More investigation is needed. The double-free is in an error-handling path from allegro4, but something has gone wrong before, which is the actual reason. More analysis is needed.

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff72d542a in __GI_abort () at abort.c:89
#2  0x00007ffff7311c00 in __libc_message (do_abort=do_abort@entry=2, 
    fmt=fmt@entry=0x7ffff7406d78 "*** Error in `%s': %s: 0x%s ***\n") at ../sysdeps/posix/libc_fatal.c:175
#3  0x00007ffff7317fc6 in malloc_printerr (action=3, str=0x7ffff7406eb0 "double free or corruption (top)", 
    ptr=<optimized out>, ar_ptr=<optimized out>) at malloc.c:5049
#4  0x00007ffff731880e in _int_free (av=0x7ffff7638b00 <main_arena>, p=0x5555557b9110, have_lock=0) at malloc.c:3905
#5  0x00007ffff767c67f in load_file_object (f=f@entry=0x5555557b6000, size=size@entry=0) at ./src/datafile.c:1351
#6  0x00007ffff767c770 in load_datafile_callback (filename=<optimized out>, callback=0x0) at ./src/datafile.c:1397
#7  0x0000555555562e3e in init_game (map_file=0x55555556b121 "maps from datafile please") at main.c:730
#8  0x0000555555556e43 in main (argc=1, argv=0x7fffffffe028) at main.c:3091

[carstene1ns]

Yep, it is the same issue I am having.

I built debug versions of allegro4, dumb and aldmb and get the same backtrace:

*** Error in `/home/carsten/projects/alex4/src/alex4': double free or corruption (!prev): 0x00005555557c9ee0 ***

#0  0x00007ffff72c18a0 in raise () from /usr/lib/libc.so.6
#1  0x00007ffff72c2f09 in abort () from /usr/lib/libc.so.6
#2  0x00007ffff7304517 in __libc_message () from /usr/lib/libc.so.6
#3  0x00007ffff730ac84 in malloc_printerr () from /usr/lib/libc.so.6
#4  0x00007ffff730c599 in _int_free () from /usr/lib/libc.so.6
#5  0x00007ffff767e073 in load_file_object (f=f@entry=0x5555557c6dc0, size=size@entry=0)
    at /tmp/makepkg/allegro4-debug/src/allegro-4.4.2/src/datafile.c:1351
        dat = <optimized out>
        prop = {dat = 0x5555557de8b0 "", type = 1330792775}
        list = 0x5555557f67f0
        count = 208
        c = 79
        type = <optimized out>
        failed = -1
#6  0x00007ffff767e188 in load_datafile_callback (filename=0x555555570397 "data/data.dat", callback=0x5555555665a2 <my_callback>)
    at /tmp/makepkg/allegro4-debug/src/allegro-4.4.2/src/datafile.c:1397
        f = 0x5555557c6dc0
        dat = <optimized out>
        type = <optimized out>
#7  0x0000555555566b33 in init_game (map_file=0x555555570e80 "maps from datafile please") at main.c:737

Relevant allegro source: https://github.com/liballeg/allegro5/blob/4.4/src/datafile.c#L1296

[Rondom]

Yes. As I assumed, the double-free is a bug in Allegro4. In case of an error, it does a double-free.

https://github.com/liballeg/allegro5/blob/20bf75791f9e1bc6892a4d56e823f70335d18610/src/datafile.c#L1349-L1351

... but unload_datafile already frees dat here:

https://github.com/liballeg/allegro5/blob/20bf75791f9e1bc6892a4d56e823f70335d18610/src/datafile.c#L1690-L1698

So, as a next step, we would need to find out why failed is true inside load_file_object.

[Rondom]

I have done some more debugging and have found the following. How to reproduce (read, continue where I left off):

• Compile libdumb with debug-symbols (-DCMAKE_BUILD_TYPE=Debug)
• Download a version of alex4 that compiles under Linux. I used the one from Debian.
• Compile alex4 against the debug-version of libaldmb (change the makefile to link against the version with debugging symbols (with the d suffix)
• debug, I suggest to start at dat_read_mod which is the callback that is called from allegro when encountering the "MOD " object in the packfile.

Inside dumb_read_mod_quick we can see that dumb errors because of some file operations failing. This may be a symptom of something else going wrong before causing wrong offsets. I honestly do not know very much about the code or the MOD format to determine what is realistic and what not. So, I would appreciate if some other brave soul could continue from here ;-) https://github.com/kode54/dumb/blob/8430c5c81ff15468b38a542b1e27ba19c2e72768/src/it/readmod.c#L483-L491

(gdb) bt
#0  it_mod_load_sigdata (f=0x5555557e8620, restrict_=2) at /home/andreas/cvs/dumb/src/it/readmod.c:483
#1  0x00007ffff79889b9 in dumb_read_mod_quick (f=0x5555557e8620, restrict_=2) at /home/andreas/cvs/dumb/src/it/readmod.c:637
#2  0x00007ffff7987430 in dumb_read_mod (f=0x5555557e8620, restrict_=2) at /home/andreas/cvs/dumb/src/it/readmod2.c:23
#3  0x00007ffff7bd712d in dat_read_mod (f=0x5555557ef1f0, size=41768) at /home/andreas/cvs/dumb/src/allegro/datmod.c:36
#4  0x00007ffff7675630 in load_object (obj=obj@entry=0x5555557bdb00, f=f@entry=0x5555557ba000, type=1297040416) at ./src/datafile.c:1114
#5  0x00007ffff76762dd in load_file_object (f=f@entry=0x5555557ba000, size=size@entry=0) at ./src/datafile.c:1266
#6  0x00007ffff76764c7 in load_datafile_callback (filename=<optimized out>, callback=0x0) at ./src/datafile.c:1337
#7  0x0000555555566663 in init_game (map_file=0x555555570610 "maps from datafile please") at main.c:730
#8  0x000055555556df0d in main (argc=1, argv=0x7fffffffe778) at main.c:3091
#9  0x00007ffff72b4561 in __libc_start_main () from /lib/x86_64-linux-gnu/libc.so.6
#10 0x000055555555746a in _start ()
(gdb) info locals
buffer = {0 '\000', 0 '\000', 0 '\000', 0 '\000', 0 '\000'}
total_sample_size = 0
remain = 2147482563
sample_number = 24
offset = 1084
sigdata = 0x5555557e9030
n_channels = 4
i = 31
fft = 1294879534
(gdb) p sigdata->sample[sample_number].length
$1 = 6144

[kode54]

Maybe the packfile seek function needs range checking for readers like the mod reader?

[Rondom]

I found out what the issue is. Just to be sure, I dumped all the data into a file to make sure that dumb can play it when reading from a normal file (it can) and noticed that the code was failing on the first read after I inserted that code, even though it seeked back to the beginning.

It turns out that Allegro4 datafile functions only support forward seeks, but dumb does backward seeks as well. https://www.allegro.cc/manual/4/api/file-and-compression-routines/pack_fseek https://github.com/liballeg/allegro5/blob/c12b6be726f8959404734e52c36edfdd6edf2461/src/file.c#L2201-L2211

What do you think about introducing some wrapper, that wraps a DUMB_FILESYSTEM that is not seekable? This would also enable reading from pipes (#23).

[kode54]

Great, so the no-backwards-seeking file readers in DUMB were done on purpose, because someone else was pedantic for us. I'll have to add a wrapper for Allegro, and for any other reader that doesn't support the seek function. So Allegro's interface should have the seek function NULLed out, and a wrapper interface added to the file code in DUMB core itself.

[Rondom]

Yeah, random seeking cannot work for example with compressed files (one would need to re-read things from the beginning all the time), which I guess was the reason for this.

@kode54 Are you planning to work on this? Otherwise I might give it a try (not now, but probably in two weeks or so, when I have more time)

[Rondom]

I have a proof of concept fix working, expect a PR soon. The fix still does not fix Alex, because I encountered another bug #86. 😔