From 6116fdb2779c7e1a8623d0f34a48c8fed2d144c5 Mon Sep 17 00:00:00 2001 From: Sebastian Gniazdowski Date: Thu, 29 Jun 2017 11:08:02 +0200 Subject: 41375: GDBM interface bug fixes --- Src/Modules/db_gdbm.c | 156 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 93 insertions(+), 63 deletions(-) (limited to 'Src') diff --git a/Src/Modules/db_gdbm.c b/Src/Modules/db_gdbm.c index 0ab0fe725..cf1322459 100644 --- a/Src/Modules/db_gdbm.c +++ b/Src/Modules/db_gdbm.c @@ -42,7 +42,9 @@ static Param createhash( char *name, int flags ); static int append_tied_name( const char *name ); static int remove_tied_name( const char *name ); static char *unmetafy_zalloc(const char *to_copy, int *new_len); -static void set_length(char *buf, int size); +static void myfreeparamnode(HashNode hn); + +static int no_database_action = 0; /* * Make sure we have all the bits I'm using for memory mapping, otherwise @@ -72,7 +74,7 @@ static char *backtype = "db/gdbm"; */ struct gsu_scalar_ext { - struct gsu_scalar std; + struct gsu_scalar std; /* Size of three pointers */ GDBM_FILE dbf; char *dbfile_path; }; @@ -106,6 +108,7 @@ static struct paramdef patab[] = { static int bin_ztie(char *nam, char **args, Options ops, UNUSED(int func)) { + struct gsu_scalar_ext *dbf_carrier; char *resource_name, *pmname; GDBM_FILE dbf = NULL; int read_write = GDBM_SYNC, pmflags = PM_REMOVABLE; @@ -164,21 +167,17 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func)) if (!(tied_param = createhash(pmname, pmflags))) { zwarnnam(nam, "cannot create the requested parameter %s", pmname); - fdtable[gdbm_fdesc(dbf)] = FDT_UNUSED; gdbm_close(dbf); return 1; } - addmodulefd(gdbm_fdesc(dbf), FDT_MODULE); - append_tied_name(pmname); - tied_param->gsu.h = &gdbm_hash_gsu; /* Allocate parameter sub-gsu, fill dbf field. * dbf allocation is 1 to 1 accompanied by * gsu_scalar_ext allocation. */ - struct gsu_scalar_ext *dbf_carrier = (struct gsu_scalar_ext *) zalloc(sizeof(struct gsu_scalar_ext)); + dbf_carrier = (struct gsu_scalar_ext *) zalloc(sizeof(struct gsu_scalar_ext)); dbf_carrier->std = gdbm_gsu_ext.std; dbf_carrier->dbf = dbf; tied_param->u.hash->tmpdata = (void *)dbf_carrier; @@ -190,6 +189,10 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func)) resource_name = xsymlink(resource_name, 1); } dbf_carrier->dbfile_path = ztrdup(resource_name); + + addmodulefd(gdbm_fdesc(dbf), FDT_INTERNAL); + append_tied_name(pmname); + return 0; } @@ -215,8 +218,9 @@ bin_zuntie(char *nam, char **args, Options ops, UNUSED(int func)) } queue_signals(); - if (OPT_ISSET(ops,'u')) - gdbmuntie(pm); /* clear read-only-ness */ + if (OPT_ISSET(ops,'u')) { + pm->node.flags &= ~PM_READONLY; + } if (unsetparam_pm(pm, 0, 1)) { /* assume already reported */ ret = 1; @@ -270,8 +274,7 @@ bin_zgdbmpath(char *nam, char **args, Options ops, UNUSED(int func)) * * It will be left in this state if database doesn't * contain such key. That might be a drawback, maybe - * setting to empty value has sense, as no other writer - * can exist. This would remove subtle hcalloc(1) leak. + * setting to empty value has sense. */ /**/ @@ -279,7 +282,8 @@ static char * gdbmgetfn(Param pm) { datum key, content; - int ret; + int ret, umlen; + char *umkey; GDBM_FILE dbf; /* Key already retrieved? There is no sense of asking the @@ -292,13 +296,13 @@ gdbmgetfn(Param pm) * - if we are readers, we for sure have newest copy of data */ if ( pm->node.flags & PM_UPTODATE ) { - return pm->u.str ? pm->u.str : (char *) hcalloc(1); + return pm->u.str ? pm->u.str : ""; } /* Unmetafy key. GDBM fits nice into this * process, as it uses length of data */ - int umlen = 0; - char *umkey = unmetafy_zalloc(pm->node.nam,¨en); + umlen = 0; + umkey = unmetafy_zalloc(pm->node.nam,¨en); key.dptr = umkey; key.dsize = umlen; @@ -314,26 +318,26 @@ gdbmgetfn(Param pm) /* Ensure there's no leak */ if (pm->u.str) { zsfree(pm->u.str); + pm->u.str = NULL; } /* Metafy returned data. All fits - metafy * can obtain data length to avoid using \0 */ pm->u.str = metafy(content.dptr, content.dsize, META_DUP); + /* gdbm allocates with malloc */ + free(content.dptr); - /* Free key, restoring its original length */ - set_length(umkey, umlen); - zsfree(umkey); + /* Free key */ + zfree(umkey, umlen+1); /* Can return pointer, correctly saved inside hash */ return pm->u.str; } - /* Free key, restoring its original length */ - set_length(umkey, umlen); - zsfree(umkey); + /* Free key */ + zfree(umkey, umlen+1); - /* Can this be "" ? */ - return (char *) hcalloc(1); + return ""; } /**/ @@ -361,7 +365,7 @@ gdbmsetfn(Param pm, char *val) /* Database */ dbf = ((struct gsu_scalar_ext *)pm->gsu.s)->dbf; - if (dbf) { + if (dbf && no_database_action == 0) { int umlen = 0; char *umkey = unmetafy_zalloc(pm->node.nam,¨en); @@ -378,15 +382,13 @@ gdbmsetfn(Param pm, char *val) (void)gdbm_store(dbf, key, content, GDBM_REPLACE); /* Free */ - set_length(umval, umlen); - zsfree(umval); + zfree(umval, umlen+1); } else { (void)gdbm_delete(dbf, key); } /* Free key */ - set_length(umkey, key.dsize); - zsfree(umkey); + zfree(umkey, key.dsize+1); } } @@ -427,7 +429,7 @@ getgdbmnode(HashTable ht, const char *name) val_pm = (Param) zshcalloc( sizeof (*val_pm) ); val_pm->node.flags = PM_SCALAR | PM_HASHELEM; /* no PM_UPTODATE */ val_pm->gsu.s = (GsuScalar) ht->tmpdata; - ht->addnode( ht, ztrdup( name ), val_pm ); // sets pm->node.nam + ht->addnode( ht, ztrdup( name ), val_pm ); /* sets pm->node.nam */ } return (HashNode) val_pm; @@ -437,7 +439,7 @@ getgdbmnode(HashTable ht, const char *name) static void scangdbmkeys(HashTable ht, ScanFunc func, int flags) { - datum key; + datum key, prev_key; GDBM_FILE dbf = ((struct gsu_scalar_ext *)ht->tmpdata)->dbf; /* Iterate keys adding them to hash, so @@ -456,7 +458,9 @@ scangdbmkeys(HashTable ht, ScanFunc func, int flags) /* Iterate - no problem as interfacing Param * will do at most only fetches, not stores */ + prev_key = key; key = gdbm_nextkey(dbf, key); + free(prev_key.dptr); } } @@ -489,8 +493,15 @@ gdbmhashsetfn(Param pm, HashTable ht) key = gdbm_firstkey(dbf); } - /* just deleted everything, clean up */ - (void)gdbm_reorganize(dbf); + /* Just deleted everything, clean up if no new data. + * User can also reorganize via gdbmtool. */ + if (!ht || ht->hsize == 0) { + (void)gdbm_reorganize(dbf); + } + + no_database_action = 1; + emptyhashtable(pm->u.hash); + no_database_action = 0; if (!ht) return; @@ -498,9 +509,11 @@ gdbmhashsetfn(Param pm, HashTable ht) /* Put new strings into database, waiting * for their interfacing-Params to be created */ - for (i = 0; i < ht->hsize; i++) + for (i = 0; i < ht->hsize; i++) { for (hn = ht->nodes[i]; hn; hn = hn->next) { struct value v; + int umlen = 0; + char *umkey, *umval; v.isarr = v.flags = v.start = 0; v.end = -1; @@ -508,8 +521,7 @@ gdbmhashsetfn(Param pm, HashTable ht) v.pm = (Param) hn; /* Unmetafy key */ - int umlen = 0; - char *umkey = unmetafy_zalloc(v.pm->node.nam,¨en); + umkey = unmetafy_zalloc(v.pm->node.nam,¨en); key.dptr = umkey; key.dsize = umlen; @@ -517,23 +529,23 @@ gdbmhashsetfn(Param pm, HashTable ht) queue_signals(); /* Unmetafy */ - char *umval = unmetafy_zalloc(getstrvalue(&v),¨en); + umval = unmetafy_zalloc(getstrvalue(&v),¨en); /* Store */ content.dptr = umval; content.dsize = umlen; (void)gdbm_store(dbf, key, content, GDBM_REPLACE); - /* Free - unmetafy_zalloc allocates exact required - * space, however unmetafied string can have zeros - * in content, so we must first fill with non-0 bytes */ - set_length(umval, content.dsize); - zsfree(umval); - set_length(umkey, key.dsize); - zsfree(umkey); + /* Free - unmetafy_zalloc allocates + * exact required space + 1 null byte */ + zfree(umval, content.dsize+1); + zfree(umkey, key.dsize+1); unqueue_signals(); } + } + /* We reuse our hash, the input is to be deleted */ + deleteparamtable(ht); } /**/ @@ -566,14 +578,16 @@ gdbmuntie(Param pm) static void gdbmhashunsetfn(Param pm, UNUSED(int exp)) { + struct gsu_scalar_ext * gsu_ext; + gdbmuntie(pm); /* Remember custom GSU structure assigned to * u.hash->tmpdata before hash gets deleted */ - struct gsu_scalar_ext * gsu_ext = pm->u.hash->tmpdata; + gsu_ext = pm->u.hash->tmpdata; - /* Uses normal unsetter. Will delete all owned - * parameters and also hashtable. */ + /* Uses normal unsetter (because gdbmuntie is called above). + * Will delete all owned field-parameters and also hashtable. */ pm->gsu.h->setfn(pm, NULL); /* Don't need custom GSU structure with its @@ -654,13 +668,17 @@ static Param createhash( char *name, int flags ) { pm->level = locallevel; /* This creates standard hash. */ - ht = pm->u.hash = newparamtable(32, name); + ht = pm->u.hash = newparamtable(17, name); if (!pm->u.hash) { paramtab->removenode(paramtab, name); paramtab->freenode(&pm->node); - zwarnnam(name, "Out of memory when allocating hash"); + zwarnnam(name, "out of memory when allocating hash"); + return NULL; } + /* Does free Param (unsetfn is called) */ + ht->freenode = myfreeparamnode; + /* These provide special features */ ht->getnode = ht->getnode2 = getgdbmnode; ht->scantab = scangdbmkeys; @@ -699,6 +717,7 @@ static int append_tied_name( const char *name ) { static int remove_tied_name( const char *name ) { int old_len = arrlen(zgdbm_tied); + int new_len; /* Two stage, to always have arrlen() == zfree-size - 1. * Could do allocation and revert when `not found`, but @@ -721,13 +740,14 @@ static int remove_tied_name( const char *name ) { /* Second stage. Size changed? Only old_size-1 * change is possible, but.. paranoia way */ - int new_len = arrlen(zgdbm_tied); + new_len = arrlen(zgdbm_tied); if (new_len != old_len) { + char **dst; char **new_zgdbm_tied = zshcalloc((new_len+1) * sizeof(char *)); /* Copy */ p = zgdbm_tied; - char **dst = new_zgdbm_tied; + dst = new_zgdbm_tied; while (*p) { *dst++ = *p++; } @@ -746,10 +766,9 @@ static int remove_tied_name( const char *name ) { * - duplicates bufer to work on it, * - does zalloc of exact size for the new string, * - restores work buffer to original content, to restore strlen - * - * No zsfree()-confusing string will be produced. */ -static char *unmetafy_zalloc(const char *to_copy, int *new_len) { +static char * +unmetafy_zalloc(const char *to_copy, int *new_len) { char *work, *to_return; int my_new_len = 0; @@ -761,7 +780,7 @@ static char *unmetafy_zalloc(const char *to_copy, int *new_len) { /* This string can be correctly zsfree()-d */ to_return = (char *) zalloc((my_new_len+1)*sizeof(char)); - memcpy(to_return, work, sizeof(char)*my_new_len); // memcpy handles $'\0' + memcpy(to_return, work, sizeof(char)*my_new_len); /* memcpy handles $'\0' */ to_return[my_new_len]='\0'; /* Restore original strlen and correctly free */ @@ -771,16 +790,27 @@ static char *unmetafy_zalloc(const char *to_copy, int *new_len) { return to_return; } -/* - * For zsh-allocator, rest of Zsh seems to use - * free() instead of zsfree(), and such length - * restoration causes slowdown, but all is this - * way strict - correct */ -static void set_length(char *buf, int size) { - buf[size]='\0'; - while ( -- size >= 0 ) { - buf[size]=' '; +static void +myfreeparamnode(HashNode hn) +{ + Param pm = (Param) hn; + + /* Upstream: The second argument of unsetfn() is used by modules to + * differentiate "exp"licit unset from implicit unset, as when + * a parameter is going out of scope. It's not clear which + * of these applies here, but passing 1 has always worked. + */ + + /* if (delunset) */ + pm->gsu.s->unsetfn(pm, 1); + + zsfree(pm->node.nam); + /* If this variable was tied by the user, ename was ztrdup'd */ + if (pm->node.flags & PM_TIED && pm->ename) { + zsfree(pm->ename); + pm->ename = NULL; } + zfree(pm, sizeof(struct param)); } #else -- cgit 1.4.1