Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Bugs
  • Category Plugins
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by amachronic - 2021-04-06
Last edited by speachy - 2021-05-12

FS#13283 - Pictureflow segfault on targets with a portrait display orientation

Entering and exiting Pictureflow multiple times will eventually generate a segfault. This can be reproduced on the simulator quite easily. Despite the apparent ‘random’ nature of the segfault, GDB and Valgrind show it is always caused by the same line of code.

Oddly enough, it only occurs for portrait display orientations (LCD_HEIGHT > LCD_WIDTH). Weird :-/. Square and landscape displays are fine, which would explain why nobody seems to be complaining… AFAICT, portrait displays are much less common especially among the stable ports.

I tested using the Sansa Fuze+ (portrait), Sansa Clip Zip+ (square), and iPod 6g (landscape) simulators.

The bug is on master, was introduced by 45915101d according to ‘git bisect’, and I confirmed it does not appear on the preceding commit (9adfab9b2).

The problem lies in the function render_slide(), on the indicated line: 

...
        int p = (bmp->height-1-DISPLAY_OFFS) * PFREAL_ONE;
        int plim = MAX(0, p - (LCD_HEIGHT/2-1) * dy);
        pix_t *pixel = LCDADDR(x, (LCD_HEIGHT/2)-1 );

        if (alpha == 256) {
            while (p >= plim) {
                *pixel = ptr[((unsigned)p) >> PFREAL_SHIFT]; /** <<< HERE **/
                p -= dy;
                pixel -= PIXELSTEP_Y;
            }
...

I think ‘p’ must have a bad value, but it kept getting optimized out (even after recompiling with -O0) so I was unable to confirm this in GDB. I can see ‘pixel’ and ‘ptr’ and they appear sane.

Running the simulator under Valgrind is the most reliable way to reproduce the bug. Valgrind does run cleanly on 45915101d and master for square and landscape displays, so it is definitely the portrait orientation that triggers this.

Here’s the Valgrind error I got from testing on a Fuze+ simulator build of 45915101d: 

==69170== Invalid read of size 2
==69170==    at 0x4BB2F67: render_slide (pictureflow.c:2708)
==69170==    by 0x4BB30FE: render_all_slides (pictureflow.c:2848)
==69170==    by 0x4BB4BF8: pictureflow_main (pictureflow.c:3684)
==69170==    by 0x4BB4BF8: plugin_start (pictureflow.c:3870)
==69170==    by 0x4BB6998: plugin__start (plugin_crt0.c:102)
==69170==    by 0x143586: plugin_load (plugin.c:961)
==69170==    by 0x157309: ft_enter (filetree.c:652)
==69170==    by 0x150672: dirbrowse (tree.c:707)
==69170==    by 0x150E47: rockbox_browse (tree.c:1021)
==69170==    by 0x12C8E5: plugins_menu (plugin_menu.c:57)
==69170==    by 0x12B9BF: do_menu (menu.c:636)
==69170==    by 0x143EDA: miscscrn (root_menu.c:347)
==69170==    by 0x1444E6: load_screen (root_menu.c:687)
==69170==  Address 0x4392fa8 is not stack'd, malloc'd or (recently) free'd
Closed by  speachy
2021-05-12 18:24
Reason for closing:  Fixed
Admin
speachy commented on 2021-04-07 11:41

A fix I just committed (8b56476a2c) _might_ affect this. I wasn't able to trigger a crash in a m3k sim build afterwards, though I didn't try pictureflow specifically previously, or with valgrind – I'm experimenting with AddressSanitizer at the moment.

amachronic commented on 2021-04-07 15:02

Tried 8b56476a2c on the M3K native simulator, and it doesn't crash out immediately but when I tried loading up a large number of albums (~1000), I got more segfaults. It was harder to trigger this time, I had to scroll back and forth and enter/exit pictureflow several times.

The core dump points to line 581 of firmware/buflib.c. Here's Valgrind's output:

==48519== Invalid read of size 8
==48519== at 0x1604C7: buflib_alloc_ex (buflib.c:581)
==48519== by 0x4CD67E6: read_pfraw (pictureflow.c:2309)
==48519== by 0x4CD73D1: load_and_prepare_surface (pictureflow.c:2349)
==48519== by 0x4CD7BAC: load_new_slide (pictureflow.c:2460)
==48519== by 0x4CD7EFC: thread (pictureflow.c:2063)
==48519== by 0x16B847: runthread (thread-sdl.c:305)
==48519== by 0x4E47D87: ??? (in /usr/lib64/libSDL-1.2.so.0.11.4)
==48519== by 0x4E7C9DA: ??? (in /usr/lib64/libSDL-1.2.so.0.11.4)
==48519== by 0x4EA5EBD: start_thread (pthread_create.c:463)
==48519== by 0x4FB949E: clone (clone.S:95)
==48519== Address 0x40084008402e6110 is not stack'd, malloc'd or (recently) free'd

amachronic commented on 2021-04-07 17:29

I managed to trigger the original bug on 8b56476a2c under the iPod 6G simulator, as well as many random segfaults tracing back to pictureflow. I guess this is just a buffer overflow somewhere as bilgus suggested on IRC.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing