From 5f817fe9ad6cf8abad9b18733250be396b8dc6a4 Mon Sep 17 00:00:00 2001 From: Ingo Paschke Date: Thu, 28 May 2026 20:32:36 +0200 Subject: [PATCH] glyphs: code-review fixes and parse_id off-by-one - cmap_offset was calculated on every compose_glyph_name() call; calculate it once with a static function. - Drop parse_id's G_ auto-populate: an unbracketed lookup allocated the index with no matching free. Linear-scan instead when absent. - compose_glyph_name: require bufsz >= BUFSZ, build names with bounded Snprintf instead of Strcpy/Strcat, drop the dead memchr. - parse_id's permonst scan used i <= pm_count, reading one past the SYM_MON block (S_nothing) and matching it as a monster; use <. - Drop a stale NO_GLYPH empty-bucket comment from the open-addressed table. --- src/glyphs.c | 104 +++++++++++++++++++++++++++------------------------ 1 file changed, 55 insertions(+), 49 deletions(-) diff --git a/src/glyphs.c b/src/glyphs.c index 1d57a4e78..226a7f208 100644 --- a/src/glyphs.c +++ b/src/glyphs.c @@ -28,7 +28,7 @@ struct find_struct { static const struct find_struct zero_find = { 0 }; struct glyphname_hash_index_entry_t { uint32 hash; - int glyphnum; /* NO_GLYPH (==MAX_GLYPH) marks an empty bucket */ + int glyphnum; }; static struct glyphname_hash_index_entry_t *glyphname_hash_indices_ptr; static size_t glyphname_hash_indices_count; @@ -39,6 +39,7 @@ staticfn int find_glyph_in_hashtable(const char *id); staticfn int cmp_glyphname_entry(const void *a, const void *b); staticfn uint32 glyph_hash(const char *id); staticfn int compose_glyph_name(int glyph, char *buf, size_t bufsz); +staticfn int get_cmap_offset(void); staticfn void to_custom_symset_entry_callback(int glyph, struct find_struct *findwhat); staticfn int parse_id(const char *id, struct find_struct *findwhat); @@ -194,32 +195,46 @@ fix_glyphname(char *str) return str; } +/* First SYM_PCHAR index in loadsyms[], cached (0 = not computed yet; + SYM_PCHAR is never at index 0). */ +static int cached_cmap_offset = 0; + +staticfn int +get_cmap_offset(void) +{ + if (!cached_cmap_offset) { + int i; + + for (i = 0; loadsyms[i].range; i++) { + if (loadsyms[i].range == SYM_PCHAR) { + cached_cmap_offset = i; + break; + } + } + } + return cached_cmap_offset; +} + /* Build the canonical "G_xxx" identifier for the given glyph into buf. * Returns 1 if a name was produced, 0 if this glyph has no canonical name * (a few unused object indices); buf[0] is set to '\0' in that case. - * Used by parse_id's bulk-iteration paths, by find_glyph_in_hashtable to - * verify hash matches, by populate_glyphname_hash_indices to fill the table, - * and by wizcustom_glyphnames. */ + * Callers must pass a BUFSZ-sized buffer. + * Used by find_glyph_in_hashtable to verify hash matches, by the + * --dumpglyphnames path, by populate_glyphname_hash_indices to fill the + * table, and by wizcustom_glyphnames. */ staticfn int compose_glyph_name(int glyph, char *buf, size_t bufsz) { - int i, j, mnum, cmap_offset = 0; + int i, j, mnum, cmap_offset; boolean skip_base = FALSE; const char *buf2, *buf3, *buf4; char tmpbuf[4][QBUFSZ]; - if (bufsz < 2) + if (bufsz < BUFSZ) return 0; buf[0] = '\0'; tmpbuf[0][0] = tmpbuf[1][0] = tmpbuf[2][0] = tmpbuf[3][0] = '\0'; - - /* compute cmap_offset (the start of SYM_PCHAR entries in loadsyms[]) */ - i = 0; - while (loadsyms[i].range) { - if (!cmap_offset && loadsyms[i].range == SYM_PCHAR) - cmap_offset = i; - i++; - } + cmap_offset = get_cmap_offset(); if (glyph_is_monster(glyph)) { buf2 = ""; @@ -241,15 +256,11 @@ compose_glyph_name(int glyph, char *buf, size_t bufsz) } else if (glyph_is_female_pet(glyph)) { buf2 = "pet_female_"; } - Strcpy(buf, "G_"); - Strcat(buf, buf2); - Strcat(buf, buf3); + Snprintf(buf, bufsz, "G_%s%s", buf2, buf3); } else if (glyph_is_body(glyph)) { buf2 = glyph_is_body_piletop(glyph) ? "piletop_body_" : "body_"; buf3 = monsdump[glyph_to_body_corpsenm(glyph)].nm; - Strcpy(buf, "G_"); - Strcat(buf, buf2); - Strcat(buf, buf3); + Snprintf(buf, bufsz, "G_%s%s", buf2, buf3); } else if (glyph_is_statue(glyph)) { buf2 = glyph_is_fem_statue_piletop(glyph) ? "piletop_statue_of_female_" @@ -261,9 +272,7 @@ compose_glyph_name(int glyph, char *buf, size_t bufsz) ? "statue_of_male_" : ""; buf3 = monsdump[glyph_to_statue_corpsenm(glyph)].nm; - Strcpy(buf, "G_"); - Strcat(buf, buf2); - Strcat(buf, buf3); + Snprintf(buf, bufsz, "G_%s%s", buf2, buf3); } else if (glyph_is_object(glyph)) { i = glyph_to_obj(glyph); if (((i > SCR_STINKING_CLOUD) && (i < SCR_MAIL)) @@ -290,12 +299,10 @@ compose_glyph_name(int glyph, char *buf, size_t bufsz) : obj_descr[i].oc_name ? obj_descr[i].oc_name : obj_descr[i].oc_descr; - Strcpy(buf, "G_"); - if (glyph_is_normal_piletop_obj(glyph) - || glyph_is_piletop_generic_obj(glyph)) - Strcat(buf, "piletop_"); - Strcat(buf, buf2); - Strcat(buf, buf3); + Snprintf(buf, bufsz, "G_%s%s%s", + (glyph_is_normal_piletop_obj(glyph) + || glyph_is_piletop_generic_obj(glyph)) ? "piletop_" : "", + buf2, buf3); } else if (glyph_is_cmap(glyph) || glyph_is_cmap_zap(glyph) || glyph_is_swallow(glyph) || glyph_is_explosion(glyph)) { int cmap = -1; @@ -368,10 +375,8 @@ compose_glyph_name(int glyph, char *buf, size_t bufsz) j = glyph - GLYPH_SWALLOW_OFF; cmap = glyph_to_swallow(glyph); mnum = j / ((S_sw_br - S_sw_tl) + 1); - Strcpy(tmpbuf[3], "swallow "); - Strcat(tmpbuf[3], monsdump[mnum].nm); - Strcat(tmpbuf[3], " "); - Strcat(tmpbuf[3], swallow_texts[cmap]); + Snprintf(tmpbuf[3], sizeof tmpbuf[3], "swallow %s %s", + monsdump[mnum].nm, swallow_texts[cmap]); buf3 = tmpbuf[3]; skip_base = TRUE; } else if (glyph_is_explosion(glyph)) { @@ -401,16 +406,13 @@ compose_glyph_name(int glyph, char *buf, size_t bufsz) if (cmap >= 0 && cmap < MAXPCHARS) buf3 = loadsyms[cmap + cmap_offset].name + 2; } - Strcpy(buf, "G_"); - Strcat(buf, buf2); - Strcat(buf, buf3); - Strcat(buf, buf4); + Snprintf(buf, bufsz, "G_%s%s%s", buf2, buf3, buf4); } else if (glyph_is_invisible(glyph)) { - Strcpy(buf, "G_invisible"); + Snprintf(buf, bufsz, "G_invisible"); } else if (glyph_is_nothing(glyph)) { - Strcpy(buf, "G_nothing"); + Snprintf(buf, bufsz, "G_nothing"); } else if (glyph_is_unexplored(glyph)) { - Strcpy(buf, "G_unexplored"); + Snprintf(buf, bufsz, "G_unexplored"); } else if (glyph_is_warning(glyph)) { j = glyph - GLYPH_WARNING_OFF; Snprintf(buf, bufsz, "G_%s%d", "warning", j); @@ -418,11 +420,6 @@ compose_glyph_name(int glyph, char *buf, size_t bufsz) if (buf[0] == '\0') return 0; - if (memchr(buf, '\0', bufsz) == NULL) { - /* defensive: caller passed an undersized buffer */ - buf[bufsz - 1] = '\0'; - return 0; - } fix_glyphname(buf + 2); nhUse(mnum); return 1; @@ -1036,10 +1033,8 @@ parse_id( } } if (is_G && id) { - /* Populate the hash table lazily, on first G_xxx lookup. */ - if (!glyphname_hash_indices_ptr) - populate_glyphname_hash_indices(); if (glyphname_hash_indices_ptr) { + /* Fast path: bsearch the populated index. */ int val = find_glyph_in_hashtable(id); if (val >= 0) { @@ -1048,6 +1043,17 @@ parse_id( findwhat->loadsyms_offset = 0; return 1; } + } else { + /* Unpopulated: linear scan, no alloc. */ + for (glyph = 0; glyph < MAX_GLYPH; ++glyph) { + if (compose_glyph_name(glyph, buf, sizeof buf) + && !strcmpi(id, buf)) { + findwhat->findtype = find_glyph; + findwhat->val = glyph; + findwhat->loadsyms_offset = 0; + return 1; + } + } } return 0; } @@ -1079,7 +1085,7 @@ parse_id( } } /* permonst entries */ - for (i = 0; i <= pm_count; ++i) { + for (i = 0; i < pm_count; ++i) { if (!strcmpi(loadsyms[i + pm_offset].name + 2, id + 2)) { findwhat->findtype = find_pm; findwhat->val = i + 1; /* starts at 1 */