summaryrefslogtreecommitdiffstats
path: root/apps
diff options
context:
space:
mode:
authorChristian Soffke <christian.soffke@gmail.com>2022-01-01 14:53:44 +0100
committerWilliam Wilgus <me.theuser@yahoo.com>2022-01-04 18:02:12 -0500
commitdded97be3413efd8d816bf3cffa2aeb9b323216f (patch)
treef185033532d75bdf28383642b1d5f00ef2e477e0 /apps
parente3b8b7fa80320f0c8bbc84d4364ea83b5991db20 (diff)
downloadrockbox-dded97be3413efd8d816bf3cffa2aeb9b323216f.tar.gz
rockbox-dded97be3413efd8d816bf3cffa2aeb9b323216f.zip
PictureFlow: Fix buffer overflow
create_track_index appears to have relied on buflib_buffer_out returning a certain amount of space without checking that it was actually available. In at least one test case, as little as 16 bytes were returned, leading to a buffer overflow and later a segfault. Change-Id: Ic0783f3cd5bf015803b7ce90537ba38ab3434bea
Diffstat (limited to 'apps')
-rw-r--r--apps/plugins/pictureflow/pictureflow.c227
1 files changed, 126 insertions, 101 deletions
diff --git a/apps/plugins/pictureflow/pictureflow.c b/apps/plugins/pictureflow/pictureflow.c
index d2b9ae5190..7801377bda 100644
--- a/apps/plugins/pictureflow/pictureflow.c
+++ b/apps/plugins/pictureflow/pictureflow.c
@@ -330,6 +330,7 @@ struct pf_track_t {
int list_y;
int list_h;
size_t borrowed;
+ size_t used;
struct track_data *index;
char *names;
};
@@ -1665,13 +1666,109 @@ static int compare_tracks (const void *a_v, const void *b_v)
return (int)(a - b);
}
+
+
+static bool track_buffer_avail(size_t needed)
+{
+ size_t total_out = 0;
+ size_t out = 0;
+ if (pf_tracks.borrowed == 0 && pf_tracks.used == 0)
+ {
+ pf_tracks.names = rb->buflib_buffer_out(&buf_ctx, &out);
+ pf_tracks.borrowed = out;
+ }
+
+ if (needed <= pf_tracks.borrowed - pf_tracks.used)
+ return true;
+
+ while (needed > (pf_tracks.borrowed + total_out) - pf_tracks.used)
+ {
+ if (!free_slide_prio(0))
+ break;
+ out = 0;
+ rb->buflib_buffer_out(&buf_ctx, &out);
+ total_out += out;
+ }
+ pf_tracks.borrowed += total_out;
+
+ // have to move already stored track_data structs
+ if (pf_tracks.count)
+ {
+ struct track_data *new_tracks = (struct track_data *)(total_out + (uintptr_t)pf_tracks.index);
+ unsigned int bytes = pf_tracks.count * sizeof(struct track_data);
+ rb->memmove(new_tracks, pf_tracks.index, bytes);
+ }
+
+ if (needed > pf_tracks.borrowed - pf_tracks.used)
+ return false;
+
+ return true;
+}
+
+
+static int pf_tcs_retrieve_track_title(int string_index, int disc_num, int track_num)
+{
+ char file_name[MAX_PATH];
+ char *track_title = NULL;
+ int str_len;
+
+ if (rb->strcmp(UNTAGGED, tcs.result) == 0)
+ {
+ /* show filename instead of <untaggged> */
+ if (!rb->tagcache_retrieve(&tcs, tcs.idx_id, tag_filename,
+ file_name, MAX_PATH))
+ return 0;
+ track_title = file_name;
+ if (track_title)
+ {
+ /* if filename remove the '/' */
+ track_title = rb->strrchr(track_title, PATH_SEPCH);
+ if (track_title)
+ track_title++;
+ }
+ }
+
+ if (!track_title)
+ track_title = tcs.result;
+
+ int max_len = rb->strlen(track_title) + 10;
+ if (!track_buffer_avail(max_len))
+ return 0;
+
+ if (track_num > 0)
+ {
+ if (disc_num > 0)
+ str_len = rb->snprintf(pf_tracks.names + string_index, max_len,
+ "%d.%02d: %s", disc_num, track_num, track_title);
+ else
+ str_len = rb->snprintf(pf_tracks.names + string_index, max_len,
+ "%d: %s", track_num, track_title);
+ }
+ else
+ str_len = rb->snprintf(pf_tracks.names + string_index, max_len,
+ "%s", track_title);
+ return str_len;
+}
+
+#if PF_PLAYBACK_CAPABLE
+static int pf_tcs_retrieve_file_name(int fn_idx)
+{
+ if (!track_buffer_avail(MAX_PATH))
+ return 0;
+
+ rb->tagcache_retrieve(&tcs, tcs.idx_id, tag_filename,
+ pf_tracks.names + fn_idx, MAX_PATH);
+
+ return rb->strlen(pf_tracks.names + fn_idx);
+}
+#endif
+
/**
Create the track index of the given slide_index.
*/
static void create_track_index(const int slide_index)
{
buf_ctx_lock();
- char temp[MAX_PATH + 1];
if ( slide_index == pf_tracks.cur_idx )
return;
@@ -1682,124 +1779,48 @@ static void create_track_index(const int slide_index)
pf_idx.album_index[slide_index].seek);
if (pf_idx.album_index[slide_index].artist_idx >= 0)
- {
rb->tagcache_search_add_filter(&tcs, tag_albumartist,
pf_idx.album_index[slide_index].artist_seek);
- }
-
- int string_index = 0, track_num;
- int disc_num;
-
- char* result = NULL;
- size_t out = 0;
+ int string_index = 0;
pf_tracks.count = 0;
- pf_tracks.names = rb->buflib_buffer_out(&buf_ctx, &out);
- pf_tracks.borrowed += out;
- int avail = pf_tracks.borrowed;
- pf_tracks.index = (struct track_data*)(pf_tracks.names + pf_tracks.borrowed);
+
while (rb->tagcache_get_next(&tcs))
{
- result = NULL;
- if (rb->strcmp(UNTAGGED, tcs.result) == 0)
- {
- /* show filename instead of <untaggged> */
- if (!rb->tagcache_retrieve(&tcs, tcs.idx_id, tag_filename,
- temp, sizeof(temp) - 1))
- {
- goto fail;
- }
- result = temp;
- }
-
- int len = 0, fn_idx = 0;
-
- avail -= sizeof(struct track_data);
- track_num = rb->tagcache_get_numeric(&tcs, tag_tracknumber);
- disc_num = rb->tagcache_get_numeric(&tcs, tag_discnumber);
-
- if (result)
- {
- /* if filename remove the '/' */
- result = rb->strrchr(result, PATH_SEPCH);
- if (result)
- result++;
- }
-
- if (!result)
- result = tcs.result;
-
- if (disc_num < 0)
- disc_num = 0;
-retry:
- if (track_num > 0)
- {
- if (disc_num)
- fn_idx = 1 + rb->snprintf(pf_tracks.names + string_index, avail,
- "%d.%02d: %s", disc_num, track_num, result);
- else
- fn_idx = 1 + rb->snprintf(pf_tracks.names + string_index, avail,
- "%d: %s", track_num, result);
- }
- else
- {
- track_num = 0;
- fn_idx = 1 + rb->snprintf(pf_tracks.names + string_index, avail,
- "%s", result);
- }
- if (fn_idx <= 0)
+ int disc_num = rb->tagcache_get_numeric(&tcs, tag_discnumber);
+ int track_num = rb->tagcache_get_numeric(&tcs, tag_tracknumber);
+ disc_num = disc_num > 0 ? disc_num : 0;
+ track_num = track_num > 0 ? track_num : 0;
+ int fn_idx = 1 + pf_tcs_retrieve_track_title(string_index, disc_num, track_num);
+ if (fn_idx <= 1)
goto fail;
-#if PF_PLAYBACK_CAPABLE
- int remain = avail - fn_idx;
- if (remain >= MAX_PATH)
- { /* retrieve filename for building the playlist */
- rb->tagcache_retrieve(&tcs, tcs.idx_id, tag_filename,
- pf_tracks.names + string_index + fn_idx, remain);
-
- len = fn_idx + rb->strlen(pf_tracks.names + string_index + fn_idx) + 1;
- /* make sure track name and file name are really split by a \0, else
- * get_track_name might fail */
- *(pf_tracks.names + string_index + fn_idx -1) = '\0';
+ pf_tracks.used += fn_idx;
- }
- else /* request more buffer so that track and filename fit */
- len = (avail - remain) + MAX_PATH;
-#else
- len = fn_idx;
+#if PF_PLAYBACK_CAPABLE
+ int fn_len = 1 + pf_tcs_retrieve_file_name(string_index + fn_idx);
+ if (fn_len <= 1)
+ goto fail;
+ pf_tracks.used += fn_len;
#endif
- if (len > avail)
- {
- while (len > avail)
- {
- if (!free_slide_prio(0))
- goto fail;
- out = 0;
- rb->buflib_buffer_out(&buf_ctx, &out);
- avail += out;
- pf_tracks.borrowed += out;
-
- struct track_data *new_tracks;
- new_tracks = (struct track_data *)(out + (uintptr_t)pf_tracks.index);
-
- unsigned int bytes = pf_tracks.count * sizeof(struct track_data);
- if (pf_tracks.count)
- rb->memmove(new_tracks, pf_tracks.index, bytes);
- pf_tracks.index = new_tracks;
- }
- goto retry;
- }
+ if (!track_buffer_avail(sizeof(struct track_data)))
+ goto fail;
- avail -= len;
- pf_tracks.index--;
+ pf_tracks.used += sizeof(struct track_data);
+ unsigned int arr_sz = (pf_tracks.count + 1) * sizeof(struct track_data);
+ // Arrray descends from upper end of buflib-borrowed buffer.
+ pf_tracks.index = (struct track_data*)(pf_tracks.names + pf_tracks.borrowed
+ - arr_sz );
pf_tracks.index->sort = (disc_num << 24) + (track_num << 14);
pf_tracks.index->sort += pf_tracks.count;
pf_tracks.index->name_idx = string_index;
pf_tracks.index->seek = tcs.result_seek;
#if PF_PLAYBACK_CAPABLE
pf_tracks.index->filename_idx = fn_idx + string_index;
+ string_index += (fn_idx + fn_len);
+#else
+ string_index += fn_idx;
#endif
pf_tracks.count++;
- string_index += len;
}
rb->tagcache_search_finish(&tcs);
@@ -1824,6 +1845,7 @@ static inline void free_borrowed_tracks(void)
{
rb->buflib_buffer_in(&buf_ctx, pf_tracks.borrowed);
pf_tracks.borrowed = 0;
+ pf_tracks.used = 0;
pf_tracks.cur_idx = -1;
buf_ctx_unlock();
}
@@ -3955,6 +3977,9 @@ static int pictureflow_main(const char* selected_file)
pf_state = pf_idle;
pf_tracks.cur_idx = -1;
+ pf_tracks.borrowed = 0;
+ pf_tracks.used = 0;
+
extra_fade = 0;
slide_frame = 0;
step = 0;