Crash on adf_bitm.c:417 using adf_show_metadata

rvalles commented 1 year ago

adf_show_metadata crashed on the first ADF I tried. Works on other ADFs, but consistently fails with this one.

#0  __memcpy_avx_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:488
#1  0x00007f854da5212c in adfReadBitmapBlock (vol=0x55b1644fe910, nSect=948, bitm=0x0) at adf_bitm.c:417
#2  0x00007f854da51544 in adfReadBitmap (vol=0x55b1644fe910, nBlock=1758, root=0x7fff5eb1e710) at adf_bitm.c:134
#3  0x00007f854da60d9a in adfMount (dev=0x55b1644fd6e0, nPart=0, readOnly=1) at adf_vol.c:215
#4  0x000055b163b1f22c in main (argc=2, argv=0x7fff5eb1ee88) at adf_show_metadata.c:57

(different optimization level, shows memcpy call)

#0  0x00007fa987a344df in memcpy (__len=512, __src=0x7fffba1f6218, __dest=0x0) at /usr/include/bits/string_fortified.h:29
#1  adfReadBitmapBlock (vol=vol@entry=0x5612fff70910, nSect=nSect@entry=948, bitm=0x0) at adf_bitm.c:417
#2  0x00007fa987a34b5a in adfReadBitmap (vol=vol@entry=0x5612fff70910, nBlock=<optimized out>, root=root@entry=0x7fffba1f6698) at adf_bitm.c:134
#3  0x00007fa987a3c882 in adfMount (dev=<optimized out>, nPart=<optimized out>, readOnly=<optimized out>) at adf_vol.c:215
#4  0x00005612fef5d0b9 in ?? ()
#5  0x00007fa987627cd0 in __libc_start_call_main (main=main@entry=0x5612fef5d020, argc=argc@entry=2, argv=argv@entry=0x7fffba1f6de8) at ../sysdeps/nptl/libc_start_call_main.h:58
#6  0x00007fa987627d8a in __libc_start_main_impl (main=0x5612fef5d020, argc=2, argv=0x7fffba1f6de8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffba1f6dd8) at ../csu/libc-start.c:360
#7  0x00005612fef5d175 in ?? ()

This is the memcpy() at https://github.com/lclevy/ADFlib/blob/master/src/adf_bitm.c#L417

adfReadBitmapBlock() is called with NULL bitm by adfReadBitmap() at https://github.com/lclevy/ADFlib/blob/master/src/adf_bitm.c#L134

The ADF is an image of a MED 3.0 (later OctaMED) OFS floppy. It will be made available on demand.

t-w commented 1 year ago

Yes, please, provide an image, or (better) the link from where it came from, if this is the case (ie. you haven't created and/or modified the image yourself).

I have found a few MEDs 3.0 here: https://retro-commodore.eu/files/downloads/Amiga/Applications/Public%20Domain/ADF/

Is it one of those? Can you check eg. the md5sum of the one causing issues?

rvalles commented 1 year ago

Yes, please, provide an image, or (better) the link from where it came from, if this is the case (ie. you haven't created and/or modified the image yourself).

I have found a few MEDs 3.0 here: https://retro-commodore.eu/files/downloads/Amiga/Applications/Public%20Domain/ADF/

Is it one of those? Can you check eg. the md5sum of the one causing issues?

None of these. Checked all the md5sums from these MED 3 ADFs against mine.

It was ripped from an actual floppy on my A1200 using amigaXfer.

There were no i/o errors reading (although Amiga disk format's sector checksums are very weak), and amigaXfer ensures transfer integrity with crc32. Of course, there could be issues with the filesystem on the floppy.

Here's the adf (link will expire in a while): https://litter.catbox.moe/g1a30c.adf

5767d7d40bad0663c1558f8bd1796eba med_3.0.adf

t-w commented 1 year ago

OK. I got the image. Thanks.

t-w commented 1 year ago

Added a reg. test that catches this (see the commit above), in debug build (with sanitizer):

[...]/ADFlib/build/debug/regtests/Test$ ./bitmap_read_segfault.sh 
=================================================================
==148515==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000000074 at pc 0x56452f8d098b bp 0x7fffbbe71ce0 sp 0x7fffbbe71cd8
WRITE of size 4 at 0x602000000074 thread T0
    #0 0x56452f8d098a in adfReadBitmap [...]/ADFlib/src/adf_bitm.c:135
    #1 0x56452f8cd936 in adfMount [...]/ADFlib/src/adf_vol.c:215
    #2 0x56452f8c0015 in main [...]/ADFlib/regtests/Test/bitmap_read_segfault.c:50
    #3 0x7fc8304281c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #4 0x7fc830428284 in __libc_start_main_impl ../csu/libc-start.c:360
    #5 0x56452f7e43f0 in _start ([...]/ADFlib/build/debug/regtests/Test/bitmap_read_segfault+0x253f0)

0x602000000074 is located 0 bytes to the right of 4-byte region [0x602000000070,0x602000000074)
allocated by thread T0 here:
    #0 0x56452f87825f in __interceptor_malloc ([...]/ADFlib/build/debug/regtests/Test/bitmap_read_segfault+0xb925f)
    #1 0x56452f8d333f in adfBitmapAllocate [...]/ADFlib/src/adf_bitm.c:548
    #2 0x56452f8d0734 in adfReadBitmap [...]/ADFlib/src/adf_bitm.c:118
    #3 0x56452f8cd936 in adfMount [...]/ADFlib/src/adf_vol.c:215
    #4 0x56452f8c0015 in main [...]/ADFlib/regtests/Test/bitmap_read_segfault.c:50
    #5 0x7fc8304281c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58

SUMMARY: AddressSanitizer: heap-buffer-overflow [...]/ADFlib/src/adf_bitm.c:135 in adfReadBitmap
Shadow bytes around the buggy address:
  0x0c047fff7fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c047fff7fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c047fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c047fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c047fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c047fff8000: fa fa 04 fa fa fa 00 fa fa fa 00 fa fa fa[04]fa
  0x0c047fff8010: fa fa 04 fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==148515==ABORTING

t-w commented 1 year ago

The problem in general comes from the fact that the code in adfReadBitmap was trying to read all non-zero entries filling-up the array created with a fixed size calculated as follows:

static uint32_t nBlock2bitmapSize ( uint32_t nBlock )
{
    uint32_t mapSize = (uint32_t) nBlock / ( 127 * 32 );
    if ( ( nBlock % ( 127 * 32 ) ) != 0 )
        mapSize++;
    return mapSize;
}

where nBlock is:

struct AdfVolume * adfMount ( struct AdfDevice * const dev,
                              const int                nPart,
                              const BOOL               readOnly )
{
[...]
   nBlock = vol->lastBlock - vol->firstBlock + 1 - 2;

   adfReadBitmap ( vol, (uint32_t) nBlock, &root );
[...]
}

what for a 880k floppy gives 1 bitmap page.

The volume in the adf image that triggers the segfault has however:

$ build/debug/examples/adf_show_metadata regtests/Dumps/g1a30c.adf
[...]

Bitmap block pointers (bmPages) (non-zero):
  bmpages [  0 ]:               0x3cd           973
  bmpages [  1 ]:               0x3b4           948

so 2 non-zero entries within its bitmap allocation pages array...

At the moment, I am not sure what that means - either this is an error, or there are valid cases like this (this is the only one encountered so far...).

If anyone knows some details about it, esp. if this is a valid case, some insights on when it can happen (bad sector, duplication, backup?) are welcomed.

(If I get or find myself no more info about such cases, I assume that this is an invalid case. I will patch the code so that such problem will be detected and a warning will be issued).

t-w commented 1 year ago

Known links/sources with details about bitmap allocation in Amiga filesystems:

Laurent Clevy's ADF format FAQ
- see also bm_pages in the structure of rootblock

Info for newer systems ie DCFS only states that:

struct RootBlock {
    LONG    Type;           /* This is used to mark the type of this
                               block and is set to TYPE_SHORT (2) */
    ULONG   OwnKey;         /* Not used by root, must be set to 0 */
    ULONG   SeqNum;         /* Not used by root, must be set to 0 */
    ULONG   HTSize;         /* Size of the hash table in longwords,
                               must be set to 72 */
    ULONG   Reserved1;      /* reserved for future revs, must be 0 */
    LONG    Checksum;       /* balance to 0 checksum.  When all longs
                               in the block are added (ignoring carry)
                               the sum should be 0 */
    ULONG   HashTable[72];  /* hash table containing block numbers of
                               files and directories in the root */
    LONG    BitmapFlag;     /* flag to say whether the bitmap is valid
                               or not.  -1=valid. 0=invalid.  If a
                               partition is mounted (or uninhibited)
                               with BitmapFlag = 0 then the validator
                               will kick in to rebuild the bitmap */
    ULONG   BitmapKeys[25]; /* An array of block numbers for the bitmap
                               on this partition.  A block number of 0
                               indicates the end of the list. */

https://wiki.osdev.org/FFS_(Amiga) (no details there...)
"Amiga Guru Book",
- page 358, bitmap field in rootblock
- page 369, bitmap blocks

So far, I do not see any indication that a 880k floppy disk could make use of more than 1 bitmap block, ie make use of more than 1 entry in bmpages[].

Besides normal use on saving things on the disk, the bitmap block is updated automatically by Amiga's disk validator if it is invalid (most likely after interruption on writing). So the bitmap blocks itself does not really contain crucial data that cannot be reconstructed (it is like cached information about allocated/not-allocated blocks).

Such disk probably should just be repaired - bmpages[1] should be set to 0 and the bmflag (indictating whether bitmap data is valid) should be set as 'invalid' - and then updated either automatically (with DiskValidator on Amiga) or possibly with with some disk utilities.

Anyway - the code in ADFlib has to be corrected to handle such case better than with a segfault...

t-w commented 7 months ago

This is fixed with #66.

Contrary to what I wrote before, such image does not have to be repaired to use with ADFlib - the new code just ignores the records beyond what is expected for the volume's size.

Optionally, you may get a warning, if CHECK_NONZERO_BMPAGES_BEYOND_BMSIZE in adf_bitm.c is set to 1, before compilation (by default, that code is disabled).

At the moment, the new code can be taken from the devel branch, in case you want to check it out.

(There should be a new release soon).

t-w commented 3 months ago

Fix released with v0.9.0.

rvalles commented 3 months ago

Tested with the problem image. All good. :+1:

lclevy / ADFlib

Crash on adf_bitm.c:417 using adf_show_metadata #63