From 70c929179b80e0657e31558e34d2bc62e1176564 Mon Sep 17 00:00:00 2001 From: Michael Sevakis Date: Sun, 12 Mar 2017 20:59:44 -0400 Subject: Dircache: Refine name allocation and error handling. * 8 bits is enough to allow 260 character base names when five bytes is the minimum indirect storage size (0..255->5..260). * Don't truncate anything that's too long as that can lead to bad behavior, simply don't include the offending entry in the parent. * Set the .tinyname flag to 1 by default to indicate that the entry's name doesn't need freeing. Clear it only when allocating indirect storage. * Rename some things to help catch all instances Change-Id: Iff747b624acbb8e03ed26c24afdf0fc715fd9d99 --- firmware/common/dircache.c | 80 +++++++++++++++++++++++----------------- firmware/common/file_internal.c | 2 +- firmware/include/file_internal.h | 2 +- 3 files changed, 49 insertions(+), 35 deletions(-) diff --git a/firmware/common/dircache.c b/firmware/common/dircache.c index 08457f2da1..42585e121b 100644 --- a/firmware/common/dircache.c +++ b/firmware/common/dircache.c @@ -149,15 +149,18 @@ enum DCM_LAST = DCM_PROCEED, }; -#define MAX_TINYNAME sizeof (uint32_t) -#define DC_MAX_NAME MIN(MAX_NAME, UINT8_MAX) +#define MAX_TINYNAME sizeof (uint32_t) +#define NAMELEN_ADJ (MAX_TINYNAME+1) +#define DC_MAX_NAME (UINT8_MAX+NAMELEN_ADJ) +#define CE_NAMESIZE(len) ((len)+NAMELEN_ADJ) +#define NAMESIZE_CE(size) ((size)-NAMELEN_ADJ) /* Throw some warnings if about the limits if things may not work */ -#if MAX_NAME > UINT8_MAX +#if MAX_COMPNAME > UINT8_MAX+5 #warning Need more than 8 bits in name length bitfield #endif -#if DIRCACHE_LIMIT > ((1 << 24)-255) +#if DIRCACHE_LIMIT > ((1 << 24)-1+5) #warning Names may not be addressable with 24 bits #endif @@ -173,7 +176,7 @@ struct dircache_entry union { struct { uint32_t name : 24; /* indirect storage (.tinyname == 0) */ - uint32_t length : 8; /* length of name indexed by 'name' */ + uint32_t namelen : 8; /* length of name - (MAX_TINYNAME+1) */ }; unsigned char namebuf[MAX_TINYNAME]; /* direct storage (.tinyname == 1) */ }; @@ -743,7 +746,7 @@ static void entry_name_copy(char *dst, const struct dircache_entry *ce) { if (LIKELY(!ce->tinyname)) { - strmemcpy(dst, get_name(ce->name), ce->length); + strmemcpy(dst, get_name(ce->name), CE_NAMESIZE(ce->namelen)); return; } @@ -876,8 +879,8 @@ static void free_name(int nameidx, size_t size) /** * allocate and assign a name to the entry */ -static bool entry_assign_name(struct dircache_entry *ce, - const unsigned char *name, size_t size) +static int entry_assign_name(struct dircache_entry *ce, + const unsigned char *name, size_t size) { unsigned char *copyto; @@ -893,21 +896,21 @@ static bool entry_assign_name(struct dircache_entry *ce, else { if (size > DC_MAX_NAME) - size = DC_MAX_NAME; + return -ENAMETOOLONG; int nameidx = alloc_name(size); if (!nameidx) - return false; + return -ENOSPC; copyto = get_name(nameidx); ce->tinyname = 0; ce->name = nameidx; - ce->length = size; + ce->namelen = NAMESIZE_CE(size); } memcpy(copyto, name, size); - return true; + return 0; } /** @@ -915,17 +918,20 @@ static bool entry_assign_name(struct dircache_entry *ce, */ static void entry_unassign_name(struct dircache_entry *ce) { - if (!ce->tinyname) - free_name(ce->name, ce->length); + if (ce->tinyname) + return; + + free_name(ce->name, CE_NAMESIZE(ce->namelen)); + ce->tinyname = 1; } /** * assign a new name to the entry */ -static bool entry_reassign_name(struct dircache_entry *ce, - const unsigned char *newname) +static int entry_reassign_name(struct dircache_entry *ce, + const unsigned char *newname) { - size_t oldlen = ce->tinyname ? 0 : ce->length; + size_t oldlen = ce->tinyname ? 0 : CE_NAMESIZE(ce->namelen); size_t newlen = strlen(newname); if (oldlen == newlen || (oldlen == 0 && newlen <= MAX_TINYNAME)) @@ -934,7 +940,7 @@ static bool entry_reassign_name(struct dircache_entry *ce, newname, newlen); if (newlen < MAX_TINYNAME) *p = '\0'; - return true; + return 0; } /* needs a new name allocation; if the new name fits in the freed block, @@ -968,7 +974,7 @@ static int alloc_entry(struct dircache_entry **res) { dircache.last_size = 0; *res = NULL; - return 0; + return -ENOSPC; } dircache.sizeused += ENTRYSIZE; @@ -976,7 +982,7 @@ static int alloc_entry(struct dircache_entry **res) ce->next = 0; ce->up = 0; ce->down = 0; - ce->length = 0; + ce->tinyname = 1; ce->frontier = FRONTIER_SETTLED; ce->serialnum = next_serialnum(); @@ -1030,19 +1036,20 @@ static int create_entry(const char *basename, struct dircache_entry **res) { int idx = alloc_entry(res); - if (!idx) - { - logf("size limit reached (entry)"); - return 0; /* full */ - } - if (!entry_assign_name(*res, basename, strlen(basename))) + if (idx > 0) { - free_orphan_entry(NULL, *res, idx); - logf("size limit reached (name)"); - return 0; /* really full! */ + int rc = entry_assign_name(*res, basename, strlen(basename)); + if (rc < 0) + { + free_orphan_entry(NULL, *res, idx); + idx = rc; + } } + if (idx == -ENOSPC) + logf("size limit reached"); + return idx; } @@ -1304,8 +1311,15 @@ static void sab_process_sub(struct sab *sabp) } int idx = create_entry(fatentp->name, &ce); - if (!idx) + if (idx <= 0) { + if (idx == -ENAMETOOLONG) + { + /* not fatal; just don't include it */ + establish_frontier(compp->idx, FRONTIER_ZONED); + continue; + } + sabp->quit = true; break; } @@ -2328,7 +2342,7 @@ void dircache_fileop_create(struct file_base_info *dirinfop, struct dircache_entry *ce; int idx = create_entry(basename, &ce); - if (idx == 0) + if (idx <= 0) { /* failed allocation; parent cache contents are not complete */ establish_frontier(dirinfop->dcfile.idx, FRONTIER_ZONED); @@ -2427,7 +2441,7 @@ void dircache_fileop_rename(struct file_base_info *dirinfop, insert_file_entry(dirinfop, ce); /* lastly, update the entry name itself */ - if (entry_reassign_name(ce, basename)) + if (entry_reassign_name(ce, basename) == 0) { /* it's not really the same one now so re-stamp it */ dc_serial_t serialnum = next_serialnum(); @@ -2506,7 +2520,7 @@ static ssize_t get_path_sub(int idx, struct get_path_sub_data *data) if (len < 0) return -2; - cename = alloca(MAX_NAME + 1); + cename = alloca(DC_MAX_NAME + 1); entry_name_copy(cename, ce); } else /* idx < 0 */ diff --git a/firmware/common/file_internal.c b/firmware/common/file_internal.c index 8fee802f6f..a109563092 100644 --- a/firmware/common/file_internal.c +++ b/firmware/common/file_internal.c @@ -505,7 +505,7 @@ walk_path(struct pathwalk *walkp, struct pathwalk_component *compp, if (!(compp->attr & ATTR_DIRECTORY)) return -ENOTDIR; - if (len >= MAX_NAME) + if (len > MAX_COMPNAME) return -ENAMETOOLONG; /* check for "." and ".." */ diff --git a/firmware/include/file_internal.h b/firmware/include/file_internal.h index bb1236aed1..5893737833 100644 --- a/firmware/include/file_internal.h +++ b/firmware/include/file_internal.h @@ -56,7 +56,7 @@ root + 'Music' + 'Artist' + 'Album' + 'Disc N' + filename */ #define STATIC_PATHCOMP_NUM 6 -#define MAX_NAME 255 +#define MAX_COMPNAME 260 /* unsigned value that will also hold the off_t range we need without overflow */ -- cgit v1.2.3