From cb7ae38fcd051b95fcc6513f91e861902ead76a7 Mon Sep 17 00:00:00 2001 From: Christian Soffke Date: Mon, 29 Jul 2024 21:11:27 +0200 Subject: plugins: properties: fix and refactor - prevent buffer overflow that could theoretically happen if an input file's dirname were larger than sizeof(str_dirname) - remove has_pl_extension, use existing filetype_get_attr - move determination of props type to single function, and eliminate need for redundantly reading the container dir for a file Change-Id: Ice7cfcb9d891d22a8eb549439569dda126435bc2 --- apps/plugins/properties.c | 283 ++++++++++++++++++---------------------------- 1 file changed, 109 insertions(+), 174 deletions(-) (limited to 'apps/plugins/properties.c') diff --git a/apps/plugins/properties.c b/apps/plugins/properties.c index 4647216390..d3fac59bde 100644 --- a/apps/plugins/properties.c +++ b/apps/plugins/properties.c @@ -33,25 +33,17 @@ enum props_types { PROPS_DIR }; -static int props_type; - static struct gui_synclist properties_lists; static struct mp3entry id3; -static int mul_id3_count; -static int skipped_count; - -static char str_filename[MAX_PATH]; -static char str_dirname[MAX_PATH]; -static char str_size[64]; -static char str_dircount[64]; -static char str_filecount[64]; -static char str_audio_filecount[64]; -static char str_date[64]; -static char str_time[64]; - +static struct tm tm; static unsigned long display_size; static int32_t lang_size_unit; -static struct tm tm; +static int props_type, mul_id3_count, skipped_count; + +static char str_filename[MAX_PATH], str_dirname[MAX_PATH], + str_size[64], str_dircount[64], str_filecount[64], + str_audio_filecount[64], str_date[64], str_time[64]; + #define NUM_FILE_PROPERTIES 5 #define NUM_PLAYLIST_PROPERTIES (1 + NUM_FILE_PROPERTIES) @@ -78,71 +70,28 @@ static const unsigned char* const props_dir[] = ID2P(LANG_MENU_SHOW_ID3_INFO), str_audio_filecount, }; -static bool file_properties(const char* selected_file) -{ - bool found = false; - struct dirent* entry; - DIR* dir = rb->opendir(str_dirname); - if (dir) - { - while(0 != (entry = rb->readdir(dir))) - { - struct dirinfo info = rb->dir_get_info(dir, entry); - if(!rb->strcmp(entry->d_name, str_filename)) - { - display_size = human_size(info.size, &lang_size_unit); - rb->snprintf(str_size, sizeof str_size, "%lu %s", - display_size, rb->str(lang_size_unit)); - rb->gmtime_r(&info.mtime, &tm); - rb->snprintf(str_date, sizeof str_date, "%04d/%02d/%02d", - tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday); - rb->snprintf(str_time, sizeof str_time, "%02d:%02d:%02d", - tm.tm_hour, tm.tm_min, tm.tm_sec); - - if (props_type != PROPS_PLAYLIST && rb->get_metadata(&id3, -1, selected_file)) - props_type = PROPS_ID3; - found = true; - break; - } - } - rb->closedir(dir); - } - return found; -} - - -static bool dir_properties(const char* selected_file, struct dir_stats *stats, - bool (*id3_cb)(const char*)) +static bool dir_properties(const char* selected_file, struct dir_stats *stats) { - bool success; - rb->strlcpy(stats->dirname, selected_file, sizeof(stats->dirname)); - if (id3_cb) - rb->splash_progress_set_delay(HZ / 2); /* hide progress bar for 0.5s */ #ifdef HAVE_ADJUSTABLE_CPU_FREQ - rb->cpu_boost(true); + rb->cpu_boost(true); #endif - success = collect_dir_stats(stats, id3_cb); - + bool success = collect_dir_stats(stats, NULL); #ifdef HAVE_ADJUSTABLE_CPU_FREQ - rb->cpu_boost(false); + rb->cpu_boost(false); #endif - if (!success) return false; - if (!id3_cb) - { - rb->strlcpy(str_dirname, selected_file, sizeof(str_dirname)); - rb->snprintf(str_dircount, sizeof str_dircount, "%d", stats->dir_count); - rb->snprintf(str_filecount, sizeof str_filecount, "%d", stats->file_count); - rb->snprintf(str_audio_filecount, sizeof str_filecount, "%d", - stats->audio_file_count); - display_size = human_size(stats->byte_count, &lang_size_unit); - rb->snprintf(str_size, sizeof str_size, "%lu %s", display_size, - rb->str(lang_size_unit)); - } + rb->strlcpy(str_dirname, selected_file, sizeof(str_dirname)); + rb->snprintf(str_dircount, sizeof str_dircount, "%d", stats->dir_count); + rb->snprintf(str_filecount, sizeof str_filecount, "%d", stats->file_count); + rb->snprintf(str_audio_filecount, sizeof str_filecount, "%d", + stats->audio_file_count); + display_size = human_size(stats->byte_count, &lang_size_unit); + rb->snprintf(str_size, sizeof str_size, "%lu %s", display_size, + rb->str(lang_size_unit)); return true; } @@ -157,12 +106,11 @@ static const char * get_props(int selected_item, void* data, { (void)data; if (PROPS_DIR == props_type) - rb->strlcpy(buffer, selected_item >= (int)(ARRAY_SIZE(props_dir)) ? "ERROR" : - (char *) p2str(props_dir[selected_item]), buffer_len); + rb->strlcpy(buffer, selected_item >= (int)(ARRAY_SIZE(props_dir)) ? + "ERROR" : (char *) p2str(props_dir[selected_item]), buffer_len); else - rb->strlcpy(buffer, selected_item >= (int)(ARRAY_SIZE(props_file)) ? "ERROR" : - (char *) p2str(props_file[selected_item]), buffer_len); - + rb->strlcpy(buffer, selected_item >= (int)(ARRAY_SIZE(props_file)) ? + "ERROR" : (char *) p2str(props_file[selected_item]), buffer_len); return buffer; } @@ -226,15 +174,13 @@ static void setup_properties_list(struct dir_stats *stats) static int browse_file_or_dir(struct dir_stats *stats) { - int button; - if (props_type == PROPS_DIR && stats->audio_file_count) rb->gui_synclist_set_nb_items(&properties_lists, NUM_AUDIODIR_PROPERTIES*2); rb->gui_synclist_draw(&properties_lists); rb->gui_synclist_speak_item(&properties_lists); while(true) { - button = rb->get_action(CONTEXT_LIST, HZ); + int button = rb->get_action(CONTEXT_LIST, HZ); /* HZ so the status bar redraws corectly */ if (rb->gui_synclist_do_button(&properties_lists, &button)) continue; @@ -257,24 +203,55 @@ static int browse_file_or_dir(struct dir_stats *stats) } } -static bool determine_file_or_dir(void) +static bool determine_props_type(const char *file) { - struct dirent* entry; - DIR* dir = rb->opendir(str_dirname); - if (dir) + if (file[0] == PATH_SEPCH) { + const char* basename = rb->strrchr(file, PATH_SEPCH) + 1; + const int dir_len = (basename - file); + if ((int) sizeof(str_dirname) <= dir_len) + return false; + rb->strlcpy(str_dirname, file, dir_len + 1); + rb->strlcpy(str_filename, basename, sizeof str_filename); + struct dirent* entry; + DIR* dir = rb->opendir(str_dirname); + if (!dir) + return false; while(0 != (entry = rb->readdir(dir))) { - if(!rb->strcmp(entry->d_name, str_filename)) + if(rb->strcmp(entry->d_name, str_filename)) + continue; + + struct dirinfo info = rb->dir_get_info(dir, entry); + if (info.attribute & ATTR_DIRECTORY) + props_type = PROPS_DIR; + else { - struct dirinfo info = rb->dir_get_info(dir, entry); - props_type = info.attribute & ATTR_DIRECTORY ? PROPS_DIR : PROPS_FILE; - rb->closedir(dir); - return true; + display_size = human_size(info.size, &lang_size_unit); + rb->snprintf(str_size, sizeof str_size, "%lu %s", + display_size, rb->str(lang_size_unit)); + rb->gmtime_r(&info.mtime, &tm); + rb->snprintf(str_date, sizeof str_date, "%04d/%02d/%02d", + tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday); + rb->snprintf(str_time, sizeof str_time, "%02d:%02d:%02d", + tm.tm_hour, tm.tm_min, tm.tm_sec); + + if (rb->filetype_get_attr(entry->d_name) == FILE_ATTR_M3U) + props_type = PROPS_PLAYLIST; + else + props_type = rb->get_metadata(&id3, -1, file) ? + PROPS_ID3 : PROPS_FILE; } + rb->closedir(dir); + return true; } rb->closedir(dir); } + else if (!rb->strcmp(file, MAKE_ACT_STR(ACTIVITY_DATABASEBROWSER))) + { + props_type = PROPS_MUL_ID3; + return true; + } return false; } @@ -287,40 +264,34 @@ bool mul_id3_add(const char *file_name) collect_id3(&id3, mul_id3_count == 0); mul_id3_count++; } - return true; } -static bool has_pl_extension(const char* filename) -{ - char *dot = rb->strrchr(filename, '.'); - return (dot && (!rb->strcasecmp(dot, ".m3u") || !rb->strcasecmp(dot, ".m3u8"))); -} - -/* Assemble track info from a database table, the contents of a playlist file, or a dir */ +/* Assemble track info from a dir, a playlist, or a database table */ static bool assemble_track_info(const char *filename, struct dir_stats *stats) { - if (!filename) - props_type = PROPS_MUL_ID3; - mul_id3_count = skipped_count = 0; - + if (props_type == PROPS_DIR) + { +#ifdef HAVE_ADJUSTABLE_CPU_FREQ + rb->cpu_boost(true); +#endif + rb->strlcpy(stats->dirname, filename, sizeof(stats->dirname)); + rb->splash_progress_set_delay(HZ/2); /* hide progress bar for 0.5s */ + bool success = collect_dir_stats(stats, &mul_id3_add); +#ifdef HAVE_ADJUSTABLE_CPU_FREQ + rb->cpu_boost(false); +#endif + if (!success) + return false; + } + else if(props_type == PROPS_PLAYLIST && + !rb->playlist_entries_iterate(filename, NULL, &mul_id3_add)) + return false; #ifdef HAVE_TAGCACHE - if (props_type == PROPS_MUL_ID3 && !rb->tagtree_subentries_do_action(&mul_id3_add)) + else if (props_type == PROPS_MUL_ID3 && + !rb->tagtree_subentries_do_action(&mul_id3_add)) return false; - else #endif - { - if (props_type == PROPS_DIR) - { - if (!dir_properties(filename, stats, &mul_id3_add)) - return false; - } - else if(props_type == PROPS_PLAYLIST) - { - if (!rb->playlist_entries_iterate(filename, NULL, &mul_id3_add)) - return false; - } - } if (mul_id3_count == 0) { @@ -338,84 +309,48 @@ static bool assemble_track_info(const char *filename, struct dir_stats *stats) enum plugin_status plugin_start(const void* parameter) { - int ret = 0; static struct dir_stats stats; const char *file = parameter; - if(!parameter) - return PLUGIN_ERROR; - - FOR_NB_SCREENS(i) - rb->viewportmanager_theme_enable(i, true, NULL); #ifdef HAVE_TOUCHSCREEN rb->touchscreen_set_mode(rb->global_settings->touch_mode); #endif - if (file[0] == '/') /* single file or folder selected */ + int ret = file && determine_props_type(file); + if (!ret) { - const char* file_name = rb->strrchr(file, '/') + 1; - const int dirlen = (file_name - file); - rb->strlcpy(str_dirname, file, dirlen + 1); - rb->snprintf(str_filename, sizeof str_filename, "%s", file + dirlen); + rb->splashf(0, "Could not find: %s", file ?: "(NULL)"); + rb->action_userabort(TIMEOUT_BLOCK); + return PLUGIN_OK; + } + FOR_NB_SCREENS(i) + rb->viewportmanager_theme_enable(i, true, NULL); - if(!determine_file_or_dir()) + if (props_type == PROPS_MUL_ID3) + ret = assemble_track_info(NULL, NULL); + else if (props_type != PROPS_ID3) + { + setup_properties_list(&stats); /* Show title during dir scan */ + if (props_type == PROPS_DIR) + ret = dir_properties(file, &stats); + } + if (!ret) + { + if (!stats.canceled) { - rb->splashf(0, "File/Dir not found: %s", file); + rb->splash(0, ID2P(LANG_PROPERTIES_FAIL)); /* TODO: describe error */ rb->action_userabort(TIMEOUT_BLOCK); - goto exit; - } - - if (props_type == PROPS_FILE) - { - if (has_pl_extension(file)) - props_type = PROPS_PLAYLIST; - - ret = !file_properties(file); - } - - if (props_type != PROPS_ID3) /* i.e. not handled by browse_id3 */ - { - setup_properties_list(&stats); /* Show title during dir scan */ - if (props_type == PROPS_DIR) - ret = !dir_properties(file, &stats, NULL); - } - if (ret) - { - ret = 0; - if (!stats.canceled) - { - /* TODO: describe error */ - rb->splash(0, ID2P(LANG_PROPERTIES_FAIL)); - rb->action_userabort(TIMEOUT_BLOCK); - } - goto exit; } } - /* database table selected */ - else if (rb->strcmp(file, MAKE_ACT_STR(ACTIVITY_DATABASEBROWSER)) - || !assemble_track_info(NULL, NULL)) - { - ret = -1; - goto exit; - } - - if (props_type == PROPS_ID3) + else if (props_type == PROPS_ID3) ret = rb->browse_id3(&id3, 0, 0, &tm, 1); /* Track Info for single file */ else if (props_type == PROPS_MUL_ID3) ret = rb->browse_id3(&id3, 0, 0, NULL, mul_id3_count); /* database tracks */ else if ((ret = browse_file_or_dir(&stats)) < 0) ret = assemble_track_info(file, &stats) ? /* playlist or folder tracks */ - rb->browse_id3(&id3, 0, 0, NULL, mul_id3_count) : - (stats.canceled ? 0 : -1); -exit: + rb->browse_id3(&id3, 0, 0, NULL, mul_id3_count) : + (stats.canceled ? 0 : -1); + FOR_NB_SCREENS(i) rb->viewportmanager_theme_undo(i, false); - switch (ret) - { - case 1: - return PLUGIN_USB_CONNECTED; - case -1: - return PLUGIN_ERROR; - default: - return PLUGIN_OK; - } + return ret == -1 ? PLUGIN_ERROR : ret == 1 ? PLUGIN_USB_CONNECTED : PLUGIN_OK; } -- cgit v1.2.3