FS#12558 - Some core_alloc users do not handle failures

Attached to Project: Rockbox
Opened by Lorenz (lorenzv) - Sunday, 22 January 2012, 20:19 GMT
Last edited by Boris Gjenero (dreamlayers) - Monday, 30 January 2012, 06:59 GMT
Task Type Bugs
Category Settings
Status New
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.10
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No


If loading a config file that holds all configuration settings (Settings -> Manage Settings - Save .cfg File) from Rockbox 3.9.1 in Rockbox 3.10 or the daily builds for Archos Recorder with Firmware V3, the player will not bootup correctly on the next restart and show:

I 4: IllInstr

directly at booting. I then press "On" again and get:

Stkov main

and the HD shuts down. If I press "On" again, the same loop continues.

The problem can only be resolved by plugging the player to USB, removing the rockbox directory and copying it again from the newest daily build or 3.10, then restart. It boots up fine again then. Doing the previous settings manually on 3.10 or the daily builds and then saving / loading them works fine and the player won´t have the boot-bug anymore.

I have attached the config file that creates the problem.
This task depends upon

Comment by Lorenz (lorenzv) - Sunday, 22 January 2012, 20:21 GMT
I am not sure if the attachement worked, I am attaching it again and hope it does now. Can someone confirm?
Comment by Boris Gjenero (dreamlayers) - Monday, 23 January 2012, 02:35 GMT
Thanks for posting the config. With that config, minus the contrast and wps lines, I'm able to reproduce a lockup on my V2 Recorder running 8a43603. I had to remove the "contrast: 29" line for testing, because it makes the LCD invisible on the V2 Recorder.

The problem is due to "max files in dir: 10000". It results in two allocations of 400000 bytes (AVERAGE_FILENAME_LENGTH * 10000): "playlist buf" in playlist_init() and "tree names" in tree_mem_init(). The is is too much considering that the device has only 2MB of RAM. The config works after reducing max files in dir. It's also possible to avoid the lockup by removing "cuesheet support: on", but then buffer size is 275KB and the disk is constantly active while playing.

The "max files in dir" upper limit definitely needs to depend on memory size. An allocation of 800000 bytes is not a problem on a device with 32MB of RAM, but it is catastrophic on a device with 2MB of RAM. The increment should also probably depend on memory size. The ability to go from 50 to 10000 with an increment of 50 doesn't seem right.
Comment by Lorenz (lorenzv) - Monday, 23 January 2012, 03:41 GMT
Ah, I see, this makes sense to check this before allocating! Though I seriously have no problem with the disk being on all the time, even on batteries my Rockbox plays about 5 hours continously even while the disk is always on. The other question is, even if I set the file-limit to 500, the Anti-Skip Buffer to 0 and the Glyph-Cache to 50, my Archos can only play about 20 seconds before spinning up the HD again with a 320kbit MP3. So in reverse this means, the HD is turned on and off all 20 seconds and while this might save some energy (though I doubt it because spinning-up always takes more energy than continously running), it definetely will shorten the life of the HD because turning it on and off so often definitely is worse than letting it run all the time.

I understand that it might make sense for devices with 8MB cache where the HD is then off for several minutes, but if even with the lowest File-Settings the HD turns on and off all 20 seconds on my Archos, I prefer that it is always on for the life of the HD.
Comment by Lorenz (lorenzv) - Monday, 23 January 2012, 03:46 GMT
Oh by the way, I am just noticing that I again have the settings for max-files at 10000 and my Archos is not crashing at startup with latest daily build. So I guess the "check" already has been implemented? Or maybe it was another problem in my CFG file? Because I am wondering that if it was this setting that crashes the player at boot, it should crash it now too after manually doing the same settings I had before?!
Comment by Lorenz (lorenzv) - Monday, 23 January 2012, 03:54 GMT
Ah OK, I had Cue-Sheet support turned off! I can reproduce the error when having max files in directory at 10000, Playlist-size at max AND Cuesheet-Support enabled. Then it brings up the "I 4: IllInstr" error again. If I just turn Cue-Sheet-Support off or reduce the Max files a bit, it works. But everything at max + cue-sheet-support on, leads to the boot-crash.
Comment by Boris Gjenero (dreamlayers) - Monday, 30 January 2012, 06:56 GMT
It seems core_alloc_ex() and core_alloc() failures are not handled in:
firmware/common/dircache.c except for non-transparent dircache_build()

Other callers obviously check for and respond to failure. I did not do detailed analysis or testing to verify that the response leads to a reasonable outcome. Some accept a handle value of 0 and some reject it. That's not a problem because handles are positive and errors return negative numbers.

The following users of core_alloc_maximum() do not handle failures:

There's also the situation where they get some memory but not enough for their purpose. I did not check how that is handled.