Fix tree.c->tree_get_entry_at() buffer overflow

I observed a crash on buflib>move_block
after dumping ram I noticed that the buffer for filetypes was being corrupted

tree_get_entry_at returns a entry from the buflib 'tree entry' buffer
filetree.c->ft_load writes data to this buffer before checking if it has
reached the last entry resulting in buffer overflow that overwrites the
next entry in the buffer ['filetypes']

Patch checks that the index passed to tree_get_entry_at() is in range
otherwise it returns NULL

Added checks + panic in other functions using tree_get_entry_at()
Fixed tree_lock_cache() calls in playlist and filetree

Change-Id: Ibf9e65652b4e00445e8e509629aebbcddffcfd4d

apps/filetree.c

@@ -137,8 +137,9 @@ static void check_file_thumbnails(struct tree_context* c)
     if(!dir)
         return;
     /* mark all files as non talking, except the .talk ones */
-    entries = tree_get_entries(c);
     tree_lock_cache(c);
+    entries = tree_get_entries(c);
+
     for (i=0; i < c->filesindir; i++)
     {
         if (entries[i].attr & ATTR_DIRECTORY)

apps/playlist.c

@@ -1573,10 +1573,10 @@ static int get_next_dir(char *dir, bool is_forward)
             break;
         }
         
+        tree_lock_cache(tc);
         files = tree_get_entries(tc);
         num_files = tc->filesindir;
 
-        tree_lock_cache(tc);
         for (i=0; i<num_files; i++)
         {
             /* user abort */
@@ -1666,9 +1666,10 @@ static int check_subdir_for_music(char *dir, const char *subdir, bool recurse)
         return -2;
     }
     
+    tree_lock_cache(tc);
     files = tree_get_entries(tc);
     num_files = tc->filesindir;
-    
+
     for (i=0; i<num_files; i++)
     {
         if (files[i].attr & ATTR_DIRECTORY)
@@ -1681,9 +1682,11 @@ static int check_subdir_for_music(char *dir, const char *subdir, bool recurse)
     }
 
     if (has_music)
+    {
+        tree_unlock_cache(tc);
         return 0;
-        
+    }
-    tree_lock_cache(tc);
+
     if (has_subdir && recurse)
     {
         for (i=0; i<num_files; i++)

apps/tree.c

@@ -22,6 +22,7 @@
 #include <stdlib.h>
 #include <stdbool.h>
 #include "string-extra.h"
+#include "panic.h"
 
 #include "applimits.h"
 #include "dir.h"
@@ -111,11 +112,12 @@ struct entry* tree_get_entries(struct tree_context *t)
 
 struct entry* tree_get_entry_at(struct tree_context *t, int index)
 {
+    if(index < 0 || index >= t->cache.max_entries)
+        return NULL; /* no entry */
     struct entry* entries = tree_get_entries(t);
     return &entries[index];
 }
 
-
 static const char* tree_get_filename(int selected_item, void *data,
                                      char *buffer, size_t buffer_len)
 {
@@ -133,9 +135,11 @@ static const char* tree_get_filename(int selected_item, void *data,
     else 
 #endif
     {
-        struct entry* e = tree_get_entry_at(local_tc, selected_item);
+        struct entry *entry = tree_get_entry_at(local_tc, selected_item);
-        name = e->name;
+        if (!entry)
-        attr = e->attr;
+            panicf("Invalid tree entry");
+        name = entry->name;
+        attr = entry->attr;
     }
     
     if(!(attr & ATTR_DIRECTORY))
@@ -175,8 +179,11 @@ static int tree_get_filecolor(int selected_item, void * data)
     if (*tc.dirfilter == SHOW_ID3DB)
         return -1;
     struct tree_context * local_tc=(struct tree_context *)data;
-    struct entry* e = tree_get_entry_at(local_tc, selected_item);
+    struct entry *entry = tree_get_entry_at(local_tc, selected_item);
-    return filetype_get_color(e->name, e->attr);
+    if (!entry)
+        panicf("Invalid tree entry");
+
+    return filetype_get_color(entry->name, entry->attr);
 }
 #endif
 
@@ -191,8 +198,11 @@ static enum themable_icons tree_get_fileicon(int selected_item, void * data)
     else
 #endif
     {
-        struct entry* e = tree_get_entry_at(local_tc, selected_item);
+        struct entry *entry = tree_get_entry_at(local_tc, selected_item);
-        return filetype_get_icon(e->attr);
+        if (!entry)
+            panicf("Invalid tree entry");
+
+        return filetype_get_icon(entry->attr);
     }
 }
 
@@ -213,9 +223,12 @@ static int tree_voice_cb(int selected_item, void * data)
     else
 #endif
     {
-        struct entry* e = tree_get_entry_at(local_tc, selected_item);
+        struct entry *entry = tree_get_entry_at(local_tc, selected_item);
-        name = e->name;
+        if (!entry)
-        attr = e->attr;
+            panicf("Invalid tree entry");
+
+        name = entry->name;
+        attr = entry->attr;
     }
     bool is_dir = (attr & ATTR_DIRECTORY);
     bool did_clip = false;
@@ -318,17 +331,22 @@ struct tree_context* tree_get_context(void)
  */
 static int tree_get_file_position(char * filename)
 {
-    int i;
+    int i, ret = -1;/* no file match, return undefined */
-    struct entry* e;
+
+    tree_lock_cache(&tc);
+    struct entry *entries = tree_get_entries(&tc);
 
     /* use lastfile to determine the selected item (default=0) */
     for (i=0; i < tc.filesindir; i++)
     {
-        e = tree_get_entry_at(&tc, i);
+        if (!strcasecmp(entries[i].name, filename))
-        if (!strcasecmp(e->name, filename))
+        {
-            return(i);
+            ret = i;
+            break;
+        }
     }
-    return(-1);/* no file can match, returns undefined */
+    tree_unlock_cache(&tc);
+    return(ret);
 }
 
 /*
@@ -527,14 +545,14 @@ char* get_current_file(char* buffer, size_t buffer_len)
         return NULL;
 #endif
 
-    struct entry* e = tree_get_entry_at(&tc, tc.selected_item);
+    struct entry *entry = tree_get_entry_at(&tc, tc.selected_item);
-    if (getcwd(buffer, buffer_len))
+    if (entry && getcwd(buffer, buffer_len))
     {
         if (tc.dirlength)
         {
             if (buffer[strlen(buffer)-1] != '/')
                 strlcat(buffer, "/", buffer_len);
-            if (strlcat(buffer, e->name, buffer_len) >= buffer_len)
+            if (strlcat(buffer, entry->name, buffer_len) >= buffer_len)
                 return NULL;
         }
         return buffer;
@@ -670,7 +688,11 @@ static int dirbrowse(void)
                 if ( numentries == 0 )
                     break;
 
-                short attr = tree_get_entry_at(&tc, tc.selected_item)->attr;
+                struct entry *entry = tree_get_entry_at(&tc, tc.selected_item);
+                if (!entry)
+                    panicf("Invalid tree entry");
+
+                short attr = entry->attr;
                 if ((tc.browse->flags & BROWSE_SELECTONLY) &&
                     !(attr & ATTR_DIRECTORY))
                 {
@@ -798,6 +820,9 @@ static int dirbrowse(void)
 #endif
                     {
                         struct entry *entry = tree_get_entry_at(&tc, tc.selected_item);
+                        if (!entry)
+                            panicf("Invalid tree entry");
+
                         attr = entry->attr;
 
                         if (currdir[1]) /* Not in / */
@@ -1017,7 +1042,7 @@ static int move_callback(int handle, void* current, void* new)
     if (cache->lock_count > 0)
         return BUFLIB_CB_CANNOT_MOVE;
 
-    size_t diff = new - current;
+    ptrdiff_t diff = (int32_t *) new - (int32_t *) current;
     /* FIX_PTR makes sure to not accidentally update static allocations */
 #define FIX_PTR(x) \
     { if ((void*)x >= current && (void*)x < (current+cache->name_buffer_size)) x+= diff; }

apps/tree.h

@@ -105,7 +105,9 @@ struct tree_context {
 /*
  * Call one of the two below after yields since the entrys may move inbetween */
 struct entry* tree_get_entries(struct tree_context *t);
+/* returns NULL on invalid index */
 struct entry* tree_get_entry_at(struct tree_context *t, int index);
+
 void tree_mem_init(void) INIT_ATTR;
 void tree_gui_init(void) INIT_ATTR;
 char* get_current_file(char* buffer, size_t buffer_len);