Database list size (e.g. artists/albums) > global_settings.max_files_in_dir

[chris_s]

So I noticed on the latest dev version, since this commit a week ago which explicitly prevents a buffer overflow, that the database will now panic when I select an item from the album or artist list with an index (e.g. the 1100th item)  apparently larger than the "max files in directory" setting.


(From apps/tree.c – this will return NULL: )

Code: [Select]

struct entry* tree_get_entry_at(struct tree_context *t, int index)
{ 
    if(index < 0 || index >= t->cache.max_entries)
        return NULL; /* no entry */

https://github.com/Rockbox/rockbox/blob/3f110daf3032187c052a6e3c1b05d01d1a4582d0/apps/tree.c#L113
 
and then panic:

Code: [Select]

struct entry *entry = tree_get_entry_at(local_tc, selected_item);
    if (!entry)
        panicf("Invalid tree entry");

I guess that makes sense because of the following line (1080) in tree_mem_init(void):

Code: [Select]

cache->max_entries = global_settings.max_files_in_dir;

My question – Is the database supposed to be constrained in this way? If so, I haven't seen this documented anywhere. The problem disappeared by increasing the "max files in dir" setting.

I never noticed this issue before, because selecting any item in the database used to (seemingly) work fine before the aforementioned commit, although I suppose the buffer still got silently corrupted?!

[Bilgus]

Thanks for the report, before it just overflowed & corrupted the bufferlib but i'll look into it 

[chris_s]

Awesome! The added bounds checking certainly made sense to me, by itself and actually brought to light the fact that I needed to increase the value for "max files in dir".

[Bilgus]

it looks like it isn't detrimental in the sense of buffer overflow.

However, dirbrowse() is receiving bad data and it'll take me some time
to look through the code and see what can/should be done about it.

[Bilgus]

Ok, got it figured out should be fixed in dev builds after 12-20-2018

The database browser uses the function dirbrowse()
dirbrowse was grabbing file attributes it didn't need in the
particular circumstance with an invalid index and thus triggered the panic

http://gerrit.rockbox.org/r/#/c/2015/

Thanks again for the report

Bilgus

[chris_s]

Wow, you're fast.  That's great. To clarify though   – looking at load_root(struct tree_context *c) in apps/tagtree.c , where the dptr pointer is incremented for as many times as the menu has entries, it still looks it will happily write to memory areas outside of the originally allocated one, if max_files_in_dir hasn't been set to a sufficiently large value. Is that a correct assumption, or am I misunderstanding things (still in the process of getting a better handle on C programming & the Rockbox codebase) ?

Code: [Select]

 struct tagentry *dptr = core_get_data(c->cache.entries_handle);
   #(...)
    for (i = 0; i < menu->itemcount; i++)
    {
        dptr->name = menu->items[i]->name;
       #(...)
     dptr++;
    }

https://github.com/Rockbox/rockbox/blob/f08d218e676f9ee8b5c26e755088671baf3a70b7/apps/tagtree.c#L1616

Code: [Select]

cache->max_entries = global_settings.max_files_in_dir;
    cache->entries_handle = core_alloc_ex("tree entries",
                                    cache->max_entries*(sizeof(struct entry)),
                                    &ops);

https://github.com/Rockbox/rockbox/blob/f08d218e676f9ee8b5c26e755088671baf3a70b7/apps/tree.c#L1083

[chris_s]

ok, I'm dumb since that's just the root menu... but looks like something similar is happening in retrieve_entries?

https://github.com/Rockbox/rockbox/blob/f08d218e676f9ee8b5c26e755088671baf3a70b7/apps/tagtree.c#L1329

[chris_s]

ugh, should have read the code.... Guess it's handled...?

Code: [Select]

 if (current_entry_count >= c->cache.max_entries)
        {
            logf("chunk mode #3: %d", current_entry_count);
            c->dirfull = true;
            sort = false;
            break ;
        }

[Bilgus]

In load_root yes it could in theory but i'm not sure how likely it is to be greater than 50 entries
still probably worthwhile to explicitly check through tree_get_entry_at()
Edit: looks like it uses a different struct

retrieve_entries looks safe it adds a few entries before checking but the min is 50 so nbd.
https://github.com/Rockbox/rockbox/blob/master/apps/tagtree.c#L1537

[chris_s]

Thanks!   

[chris_s]

Hey William,

I’ve now noticed another issue introduced (or brought to light) by this commit.

Directly after selecting another theme (while a file must be playing), you may see the entries for the list of themes vanishing or being filled  with content that obviously doesn’t belong there (see attachment) – sometimes seemingly random assortment of characters or memory content.


The problem goes away after changing line 1045 from

Code: [Select]

ptrdiff_t diff = (int32_t *) new - (int32_t *) current;

to

Code: [Select]

ptrdiff_t diff = new - current;

[Bilgus]

Huh I find it odd that casting them to int32* would cause issues how did you decide to try removing the casts?

I'll update the code

[chris_s]

Huh I find it odd that casting them to int32* would cause issues how did you decide to try removing the casts?

I'll update the code

Honestly, I just saw that there was previously no casting, and didn't see it in other places in the codebase with similar pointer arithmetic (which i really only have basic experience with). I assumed it was worth a shot after other code changes couldn't explain the effect. I (unfortunately) don't have a good explanation.

[Bilgus]

Thanks for the testing/update the new build should be up within the hour

[chris_s]

Sure thing. Thank you! In case you figure out the explanation at some point, please let me know.