Fix regression when unsetting variables.

PATCH In review Brian Koropoff 2 participants 2 messages Jun 16, 2010 – Jul 6, 2010

Share & tools

Brian Koropoff

author

2010-06-16 02:31 UTC #0 lore raw

PATCH 1/1 Fix regression when unsetting variables.

When removing a variable from its hash chain, assign to the 'next' pointer of the previous link rather than the hash bucket.

---
 src/var.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

src/var.c +3 -3

index f456fbd..c0e5706 100644
@@ -238,11 +238,11 @@ intmax_t setvarint(const char *name, intmax_t val, int flags)
 void
 setvareq(char *s, int flags)
 {
-	struct var *vp, **vpp;
+	struct var *vp, **vpp, **fvpp;
 
 	vpp = hashvar(s);
 	flags |= (VEXPORT & (((unsigned) (1 - aflag)) - 1));
-	vp = *findvar(vpp, s);
+	vp = *(fvpp = findvar(vpp, s));
 	if (vp) {
 		if (vp->flags & VREADONLY) {
 			const char *n;

@@ -265,7 +265,7 @@ setvareq(char *s, int flags)
 
 		if (((flags & (VEXPORT|VREADONLY|VSTRFIXED|VUNSET)) |
 		     (vp->flags & VSTRFIXED)) == VUNSET) {
-			*vpp = vp->next;
+			*fvpp = vp->next;
 			ckfree(vp);
 out_free:
 			if ((flags & (VTEXTFIXED|VSTACK|VNOSAVE)) == VNOSAVE)

Signature · 1 line

1.7.0.4

Herbert Xu

2010-07-06 09:42 UTC #1 lore raw

Quoted 2 lines · Brian Koropoff

When removing a variable from its hash chain, assign to the 'next' pointer of the previous link rather than the hash bucket.

OK I ended up fixing it slightly differently.

commit cb20b2cd727c892756ff3f144ebf92feb5590562

Author: Herbert Xu <[email protected]>
Date:   Tue Jul 6 17:40:53 2010 +0800

[VAR] Fix loss of variables when hash collides

Brian Koropoff reported that the new var patches broke the following script:

#!/bin/dash
GDM_LANG="bar"
OPTION="foo"
unset GDM_LANG
# OPTION has mysteriously become unset
echo "$OPTION"

He correctly diagnosed this as a result of removing all variables in the hash chain preceding the one that should be removed in setvareq.

He also provided a patch to fix this.

This patch is based on his but without keeping the original vpp. As a result, we now store new variables at the end of the hash chain instead of the beginning.

To make this work, setvareq/setvar now returns the vp pointer modified. In case they're used to unset a variable the pointer returned is undefined. This is because mklocal needs it and used to get it by assuming that the new variable always appear at the beginning of the chain.

Signed-off-by Herbert Xu <[email protected]>

ChangeLog +4 -0

index e45405c..48436b2 100644
@@ -1,3 +1,7 @@
+2010-07-06  Herbert Xu <[email protected]>
+
+	* Fix loss of variables when hash collides.
+
 2010-06-28  Gerrit Pape <[email protected]>
 
 	* Don't clear eflag in evalbackcmd.

src/var.c +16 ~~-12~~

index f456fbd..3efc943 100644
@@ -180,13 +180,13 @@ initvar(void)
  * flags of the variable.  If val is NULL, the variable is unset.
  */
 
-void
-setvar(const char *name, const char *val, int flags)
+struct var *setvar(const char *name, const char *val, int flags)
 {
 	char *p, *q;
 	size_t namelen;
 	char *nameeq;
 	size_t vallen;
+	struct var *vp;
 
 	q = endofname(name);
 	p = strchrnul(q, '=');

@@ -206,8 +206,10 @@ setvar(const char *name, const char *val, int flags)
 		p = mempcpy(p, val, vallen);
 	}
 	*p = '\0';
-	setvareq(nameeq, flags | VNOSAVE);
+	vp = setvareq(nameeq, flags | VNOSAVE);
 	INTON;
+
+	return vp;
 }
 
 /*

@@ -235,14 +237,14 @@ intmax_t setvarint(const char *name, intmax_t val, int flags)
  * Called with interrupts off.
  */
 
-void
-setvareq(char *s, int flags)
+struct var *setvareq(char *s, int flags)
 {
 	struct var *vp, **vpp;
 
 	vpp = hashvar(s);
 	flags |= (VEXPORT & (((unsigned) (1 - aflag)) - 1));
-	vp = *findvar(vpp, s);
+	vpp = findvar(vpp, s);
+	vp = *vpp;
 	if (vp) {
 		if (vp->flags & VREADONLY) {
 			const char *n;

@@ -255,7 +257,7 @@ setvareq(char *s, int flags)
 		}
 
 		if (flags & VNOSET)
-			return;
+			goto out;
 
 		if (vp->func && (flags & VNOFUNC) == 0)
 			(*vp->func)(strchrnul(s, '=') + 1);

@@ -270,13 +272,13 @@ setvareq(char *s, int flags)
 out_free:
 			if ((flags & (VTEXTFIXED|VSTACK|VNOSAVE)) == VNOSAVE)
 				ckfree(s);
-			return;
+			goto out;
 		}
 
 		flags |= vp->flags & ~(VTEXTFIXED|VSTACK|VNOSAVE|VUNSET);
 	} else {
 		if (flags & VNOSET)
-			return;
+			goto out;
 		if ((flags & (VEXPORT|VREADONLY|VSTRFIXED|VUNSET)) == VUNSET)
 			goto out_free;
 		/* not found */

@@ -289,6 +291,9 @@ out_free:
 		s = savestr(s);
 	vp->text = s;
 	vp->flags = flags;
+
+out:
+	return vp;
 }

@@ -486,10 +491,9 @@ void mklocal(char *name)
 		eq = strchr(name, '=');
 		if (vp == NULL) {
 			if (eq)
-				setvareq(name, VSTRFIXED);
+				vp = setvareq(name, VSTRFIXED);
 			else
-				setvar(name, NULL, VSTRFIXED);
-			vp = *vpp;	/* the new variable */
+				vp = setvar(name, NULL, VSTRFIXED);
 			lvp->flags = VUNSET;
 		} else {
 			lvp->text = vp->text;

src/var.h +2 -2

index 7e7e505..7aa051f 100644
@@ -128,9 +128,9 @@ extern const char defpathvar[];
 #define mpathset()	((vmpath.flags & VUNSET) == 0)
 
 void initvar(void);
-void setvar(const char *, const char *, int);
+struct var *setvar(const char *name, const char *val, int flags);
 intmax_t setvarint(const char *, intmax_t, int);
-void setvareq(char *, int);
+struct var *setvareq(char *s, int flags);
 struct strlist;
 void listsetvar(struct strlist *, int);
 char *lookupvar(const char *);

Thanks,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt