41375: GDBM interface bug fixes

2 files changed, 97 insertions, 63 deletions

diff --git a/ChangeLog b/ChangeLog
index 0f0d6d31b..fb79b29f9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2017-07-03  Peter Stephenson  <p.stephenson@samsung.com>
+
+	* Sebastian: 41375: Src/Modules/db_gdbm.c: GDBM interface bug fixes.
+
 2017-07-02  Peter Stephenson  <p.w.stephenson@ntlworld.com>
 
 	* 41386: Src/jobs.c: when backgrounding a STAT_CURSH job, remove
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,&umlen);
+    umlen = 0;
+    umkey = unmetafy_zalloc(pm->node.nam,&umlen);
 
     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,&umlen);
 
@@ -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,&umlen);
+            umkey = unmetafy_zalloc(v.pm->node.nam,&umlen);
 
 	    key.dptr = umkey;
 	    key.dsize = umlen;
@@ -517,23 +529,23 @@ gdbmhashsetfn(Param pm, HashTable ht)
 	    queue_signals();
 
             /* Unmetafy */
-            char *umval = unmetafy_zalloc(getstrvalue(&v),&umlen);
+            umval = unmetafy_zalloc(getstrvalue(&v),&umlen);
 
             /* 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