IFS and read builtin

Hi, as has been reported already dash currently has a bug where the read builtin ignores the read environment's IFS setting. As a result,

echo a:b | { IFS=: read a b; echo $a; }

will write out a:b. I tried to see what changed between 0.5.5.1 and 0.5.6, and found that the old code used bltinlookup("IFS"). So, I made the new code also use that. The above example works properly with the attached patch.

However, testing further, I found that there is bad interaction (even without my patch, where the IFS assignment below should just be ignored) between the code that handles variable assignments, and read. Try this:

   $ src/dash -c 'printf "a\t\tb\n" | { IFS=$(printf "\t") read a b c; echo "|$a|$b|$c|"; }'
   |a||b|
   $ src/dash -c 'printf "a\t\tb\n" | { IFS="$(printf "\t")" read a b c; echo "|$a|$b|$c|"; }'
   |a|b||

This happens because expbackq is correctly called without EXP_QUOTED, which makes it call recordregion, which isn't cleared by the time read calls ifsbreakup. I'm not sure how that should be solved, and if there are more cases where it would fail. The simplest way to solve this for read is to call removerecordregions(0) before recordregion(0, len - 1, 0). I have tested that locally, and it works. I am not familiar enough with the code to judge whether the same situation can also happen in other cases that would also need fixing, which is why I have not included that part in the attached patch.

Cheers,
Harald

(Re-sent because of local mail problems.)

index f2f964c..3ba1a38 100644
@@ -205,7 +205,7 @@ expandarg(union node *arg, struct arglist *arglist, int flag)
 	 * TODO - EXP_REDIR
 	 */
 	if (flag & EXP_FULL) {
-		ifsbreakup(p, &exparg);
+		ifsbreakup(p, &exparg, 0);
 		*exparg.lastp = NULL;
 		exparg.lastp = &exparg.list;
 		expandmeta(exparg.list, flag);

@@ -1022,9 +1022,11 @@ recordregion(int start, int end, int nulonly)
  * Break the argument string into pieces based upon IFS and add the
  * strings to the argument list.  The regions of the string to be
  * searched for IFS characters have been stored by recordregion.
+ * If bltin is set, use bltinlookup to search for IFS in the
+ * environment of the currently executing built-in command.
  */
 void
-ifsbreakup(char *string, struct arglist *arglist)
+ifsbreakup(char *string, struct arglist *arglist, int bltin)
 {
 	struct ifsregion *ifsp;
 	struct strlist *sp;

@@ -1040,7 +1042,13 @@ ifsbreakup(char *string, struct arglist *arglist)
 	if (ifslastp != NULL) {
 		ifsspc = 0;
 		nulonly = 0;
-		realifs = ifsset() ? ifsval() : defifs;
+		if (!bltin)
+			realifs = ifsset() ? ifsval() : defifs;
+		else {
+			realifs = bltinlookup("IFS");
+			if (realifs == NULL)
+				realifs = defifs;
+		}
 		ifsp = &ifsfirst;
 		do {
 			p = string + ifsp->begoff;

index 405af0b..8eb5f07 100644
@@ -69,7 +69,7 @@ char *_rmescapes(char *, int);
 int casematch(union node *, char *);
 void recordregion(int, int, int);
 void removerecordregions(int); 
-void ifsbreakup(char *, struct arglist *);
+void ifsbreakup(char *, struct arglist *, int bltin);
 
 /* From arith.y */
 intmax_t arith(const char *);

index 5ab1648..898ab09 100644
@@ -87,7 +87,7 @@ readcmd_handle_line(char *line, char **ap, size_t len)
 	arglist.lastp = &arglist.list;
 	recordregion(0, len - 1, 0);
 	
-	ifsbreakup(s, &arglist);
+	ifsbreakup(s, &arglist, 1);
 	*arglist.lastp = NULL;
 	removerecordregions(0);
 

This has already been fixed in a totally different way in master. See git commits near 95a60b2936e8835963bfb08eadc0edf9dddf0498.

Ick. Your change will probably work, but it remains sneaky action at a distance. To reduce complexity, it would be good to implement read's splitting without using expand.c. I estimate the extra lines of code from importing FreeBSD's code at less than 50. It will also fix an edge case with the splitting. The last two lines of FreeBSD's builtins/read1.0 test are:

echo " 1 ,2 3," | { IFS=', ' read a b c; echo "x${a}x${b}x${c}x"; }
echo " 1 ,2 3,," | { IFS=', ' read a b c; echo "x${a}x${b}x${c}x"; }

These should result in:

x1x2x3x
x1x2x3,,x

bash and ksh93 agree on this. However, dash master prints:

x1x2x3,x
x1x2x3,,x

Jilles Tjoelker

Interesting, thank you for the reference. That commit (from May) does a lot more than fix the read problem, and does not specifically address the read problem at all, which is probably why it has not made it into 0.5.6.1 (from June), and why I had not found it.

The IFS problem was breaking the build of glibc, and reportedly also causing an unbootable system for someone because of a bad build of libusb, so it was important to fix it somehow for Gentoo. Would you prefer I continue to use a version of the patch I posted, or use the more invasive commits from master?

read used to have its own splitting code, but it was changed to use expand.c some time between 0.5.5 and 0.5.6.1. The old splitting code had bugs that expand.c did not have.

This is also what zsh prints. Could you explain why bash and ksh are correct? By my reading, when IFS=', ', then splitting " 1 ,2 3," results in "1", "2", "3", and "". Leading and trailing IFS white space gets ignored, but the trailing , is not white space, so makes the total field count 4. Since there are four fields, a is assigned "1", b is assigned "2", c is assigned "3," which is "3", the separator ",", and the final field "". But I'll be the first to admit I'm far from an expert, and I trust bash to get these things right, I'm just curious why bash is right.

That said, zsh is consistent here, and dash is not. zsh also makes
x=" 1 ,2 3,"
IFS=", "
set $x
echo $#
print out 4, while dash prints 3 in this case. So either way, something is wrong.

I think you should do what you think is best for the stability of your product. Because dash releases are not extensively tested, I'd recommend a trial build of at least a minimal base system with the new version you choose. A particular feature to be wary of is LINENO support, as it will cause most configure scripts to accept dash as a usable shell.

Right, but it seems better to fix the bugs rather than entangle things with expand.c and still leave bugs.

I think the important thing is that IFS characters are supposed to be field terminators (see POSIX XCU 2.6.5 Field Splitting).

Therefore, in the example " 1 ,2 3," there are three fields, each containing one digit, and each variable is assigned one of them.

In the example " 1 ,2 3,," there are four fields, the last being empty. Then c is assigned the third field plus the delimiter character and the remaining fields and their delimiters except trailing whitespace that is in IFS. Hence, both commas end up in c.

Likewise, in your example with normal field splitting, dash is right and zsh is wrong.

Jilles Tjoelker

Thanks, I'm aware of that. I already locally exported CONFIG_SHELL, so that even without LINENO support the configure scripts were already run from dash.

That reminds me: the LINENO support is useful, but the tracking of line numbers has some issues:

$ src/dash -c 'f() { echo $LINENO; }
f
f
'
2
3

But this is not new, and not limited to LINENO:

$ cat >foo.sh
if :; then
foo
:
:
:
:
:
fi
$ src/dash foo.sh
foo.sh: 8: foo: not found
$ bash foo.sh
foo.sh: line 2: foo: command not found

I have a patch that improves this by storing the line numbers in the command nodes, if you're interested, but it needs polishing before I plan on sending it here or anywhere, and there are probably some corner cases that it mishandles.

Agreed, unless there's an easy way to ensure the command-line processing has finished and cleaned up completely by the time read starts.

[IFS=", "]

The more I read it, the more I'm actually becoming convinced that zsh is doing the right thing, and dash is almost doing the right thing.

2.6.5 uses the term "delimiter", not "terminator". They don't mean the same thing. A delimiter can mark the start of a field as well as the end. And if you compare susv2 with susv3, you may see susv2 is a lot clearer than v3 on one point, because it ends the "Field Splitting" section with a note.

"The last rule can be summarised as a pseudo-ERE:

    (s*ns*|s+)

where s is an white-space character and n is a character in the that is not white space. Any string matching that ERE delimits a field, except that the s+ form does not delimit fields at the beginning or the end of a line." (followed by an example)

This says the s+ form does not delimit fields at the end of a line, which strongly implies that the s*ns* form does. The wording is wrong no matter how you look at it (splitting "a " results in one field "a", not one field "a "), and the note has been removed in susv3. Still, it manages to somewhat clarify the rest of text.

The read command description states:

"If there are fewer var operands specified than there are fields, the
leftover fields and their intervening separators shall be assigned to the last var."

If " 1 ,2 3,," forms four fields, where the fourth field is the empty string between the two trailing commas, then the final comma is not an "intervening separator", so it should be excluded from c.

Cheers,
Harald

Yes, I think that's the proper way to implement LINENO.

FreeBSD sh avoids extending the nodes by detecting expansions of LINENO at parse time and storing the line number at that time. However, this is only possible because it does not print a line number when there is an error in a builtin.

POSIX.1-2008 (aka SUSv4) says, at one point, that the shell shall "use the delimiters as field terminators".

The specification of field splitting did indeed change at some point, so that a final non-whitespace IFS character does not have a final empty field after it. Likely, the old spec was not what was intended as the System V sh behaved much like the new spec (though not exactly). Generally, the POSIX shell command language is designed to match the Bourne shell (as in System V) and ksh88, and deviations from this are mentioned in the rationale. Note that this does not mean that either the Bourne shell or ksh88 are compliant.

The description of the read utility is also different in POSIX.1-2008. It does not talk about "intervening separators" but about "delimiters" in general.

The intention is that if there are more fields than variables, the final variable receive the exact text after the already assigned fields and their delimiters (apart from trailing IFS whitespace). The POSIX.1-2008 text achieves this if used with the POSIX.1-2008 field splitting rules, and so does the text you cited if used with the old field splitting rules (which result in five fields for " 1 ,2 3,,").

Jilles Tjoelker

I'm curious to try to see how FreeBSD sh handles eval 'echo $LINENO', then. This will be a nice excuse to give it another try.

[...]

Ah! So something _was_ wrong -- the standard I was reading. That explains it all, thank you very much.

When I have a more polished and complete LINENO implementation, I'll post it here.

Cheers,
Harald

I agree that dash is wrong here. However, the underlying logic is correct as you can see from:

$ echo "1,2,3,"         | { read a; IFS=','; set -- $a; echo $#; }
3
$

So it's just the use in read that needs to be fixed.

FWIW I tried your patch but it grew the binary by about 0.5%. This coupled with the fact that we'll now need to maintain two copies of IFS logic (should it ever change again god forbid) means that I'd like to stay with the current implementation if possible.

Thanks,

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

This problem should be fixed in the main trunk. I will make a new release soon.

Thanks for catching this.

This is the result of ifslastp leaking out of expandarg which isn't supposed to happen. The following patch should fix it plus a real memory leak that occurs if we bail before hitting ifsfree.

commit 5c7042771753d5a968b2b7263cf9f4e02fa3820e

Author: Herbert Xu <[email protected]>
Date:   Wed Sep 8 19:51:10 2010 +0800

[EXPAND] Fix ifsfirst/ifslastp leak

As it stands expandarg may return with a non-NULL ifslastp which then confuses any subsequent ifsbreakup user that doesn't clear it directly.

What's worse, if we get interrupted before we hit ifsfree in expandarg we will leak memory.

This patch fixes this by always calling ifsfree in expandarg thus ensuring that ifslastp is always NULL on the normal path. It also adds an ifsfree call to the RESET path to ensure that memory isn't leaked.

index 3c26149..a51975c 100644
@@ -1,3 +1,7 @@
+2010-09-08  Herbert Xu <[email protected]>
+
+	* Fix ifsfirst/ifslastp leak.
+
 2010-09-08  maximilian attems <[email protected]>
 
 	* Debug compile fix.

index f2f964c..d6c6416 100644
@@ -117,7 +117,6 @@ STATIC char *evalvar(char *, int);
 STATIC size_t strtodest(const char *, const char *, int);
 STATIC void memtodest(const char *, size_t, const char *, int);
 STATIC ssize_t varvalue(char *, int, int);
-STATIC void ifsfree(void);
 STATIC void expandmeta(struct strlist *, int);
 #ifdef HAVE_GLOB
 STATIC void addglob(const glob_t *);

@@ -191,8 +190,6 @@ expandarg(union node *arg, struct arglist *arglist, int flag)
 
 	argbackq = arg->narg.backquote;
 	STARTSTACKSTR(expdest);
-	ifsfirst.next = NULL;
-	ifslastp = NULL;
 	argstr(arg->narg.text, flag);
 	p = _STPUTC('\0', expdest);
 	expdest = p - 1;

@@ -215,8 +212,7 @@ expandarg(union node *arg, struct arglist *arglist, int flag)
 		*exparg.lastp = sp;
 		exparg.lastp = &sp->next;
 	}
-	if (ifsfirst.next)
-		ifsfree();
+	ifsfree();
 	*exparg.lastp = NULL;
 	if (exparg.list) {
 		*arglist->lastp = exparg.list;

@@ -1108,22 +1104,25 @@ add:
 	arglist->lastp = &sp->next;
 }
 
-STATIC void
-ifsfree(void)
+void ifsfree(void)
 {
-	struct ifsregion *p;
+	struct ifsregion *p = ifsfirst.next;
+
+	if (!p)
+		goto out;
 
 	INTOFF;
-	p = ifsfirst.next;
 	do {
 		struct ifsregion *ifsp;
 		ifsp = p->next;
 		ckfree(p);
 		p = ifsp;
 	} while (p);
-	ifslastp = NULL;
 	ifsfirst.next = NULL;
 	INTON;
+
+out:
+	ifslastp = NULL;
 }
 
 

@@ -1678,7 +1677,6 @@ casematch(union node *pattern, char *val)
 	setstackmark(&smark);
 	argbackq = pattern->narg.backquote;
 	STARTSTACKSTR(expdest);
-	ifslastp = NULL;
 	argstr(pattern->narg.text, EXP_TILDE | EXP_CASE);
 	STACKSTRNUL(expdest);
 	result = patmatch(stackblock(), val);

@@ -1718,3 +1716,13 @@ varunset(const char *end, const char *var, const char *umsg, int varflags)
 	}
 	sh_error("%.*s: %s%s", end - var - 1, var, msg, tail);
 }
+
+#ifdef mkinit
+
+INCLUDE "expand.h"
+
+RESET {
+	ifsfree();
+}
+
+#endif

index 405af0b..4251a4a 100644
@@ -70,6 +70,7 @@ int casematch(union node *, char *);
 void recordregion(int, int, int);
 void removerecordregions(int); 
 void ifsbreakup(char *, struct arglist *);
+void ifsfree(void);
 
 /* From arith.y */
 intmax_t arith(const char *);

Cheers,

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

Slight update, should also call ifsfree instead of removerecordregion in read(1).

commit f42e443bb511ed3224f09b4fcf0772438ebdbbfa

Author: Herbert Xu <[email protected]>
Date:   Wed Sep 8 20:07:26 2010 +0800

[EXPAND] Fix ifsfirst/ifslastp leak

As it stands expandarg may return with a non-NULL ifslastp which then confuses any subsequent ifsbreakup user that doesn't clear it directly.

What's worse, if we get interrupted before we hit ifsfree in expandarg we will leak memory.

This patch fixes this by always calling ifsfree in expandarg thus ensuring that ifslastp is always NULL on the normal path. It also adds an ifsfree call to the RESET path to ensure that memory isn't leaked.

index 3c26149..a51975c 100644
@@ -1,3 +1,7 @@
+2010-09-08  Herbert Xu <[email protected]>
+
+	* Fix ifsfirst/ifslastp leak.
+
 2010-09-08  maximilian attems <[email protected]>
 
 	* Debug compile fix.

index f2f964c..d6c6416 100644
@@ -117,7 +117,6 @@ STATIC char *evalvar(char *, int);
 STATIC size_t strtodest(const char *, const char *, int);
 STATIC void memtodest(const char *, size_t, const char *, int);
 STATIC ssize_t varvalue(char *, int, int);
-STATIC void ifsfree(void);
 STATIC void expandmeta(struct strlist *, int);
 #ifdef HAVE_GLOB
 STATIC void addglob(const glob_t *);

@@ -191,8 +190,6 @@ expandarg(union node *arg, struct arglist *arglist, int flag)
 
 	argbackq = arg->narg.backquote;
 	STARTSTACKSTR(expdest);
-	ifsfirst.next = NULL;
-	ifslastp = NULL;
 	argstr(arg->narg.text, flag);
 	p = _STPUTC('\0', expdest);
 	expdest = p - 1;

@@ -215,8 +212,7 @@ expandarg(union node *arg, struct arglist *arglist, int flag)
 		*exparg.lastp = sp;
 		exparg.lastp = &sp->next;
 	}
-	if (ifsfirst.next)
-		ifsfree();
+	ifsfree();
 	*exparg.lastp = NULL;
 	if (exparg.list) {
 		*arglist->lastp = exparg.list;

@@ -1108,22 +1104,25 @@ add:
 	arglist->lastp = &sp->next;
 }
 
-STATIC void
-ifsfree(void)
+void ifsfree(void)
 {
-	struct ifsregion *p;
+	struct ifsregion *p = ifsfirst.next;
+
+	if (!p)
+		goto out;
 
 	INTOFF;
-	p = ifsfirst.next;
 	do {
 		struct ifsregion *ifsp;
 		ifsp = p->next;
 		ckfree(p);
 		p = ifsp;
 	} while (p);
-	ifslastp = NULL;
 	ifsfirst.next = NULL;
 	INTON;
+
+out:
+	ifslastp = NULL;
 }
 
 

@@ -1678,7 +1677,6 @@ casematch(union node *pattern, char *val)
 	setstackmark(&smark);
 	argbackq = pattern->narg.backquote;
 	STARTSTACKSTR(expdest);
-	ifslastp = NULL;
 	argstr(pattern->narg.text, EXP_TILDE | EXP_CASE);
 	STACKSTRNUL(expdest);
 	result = patmatch(stackblock(), val);

@@ -1718,3 +1716,13 @@ varunset(const char *end, const char *var, const char *umsg, int varflags)
 	}
 	sh_error("%.*s: %s%s", end - var - 1, var, msg, tail);
 }
+
+#ifdef mkinit
+
+INCLUDE "expand.h"
+
+RESET {
+	ifsfree();
+}
+
+#endif

index 405af0b..4251a4a 100644
@@ -70,6 +70,7 @@ int casematch(union node *, char *);
 void recordregion(int, int, int);
 void removerecordregions(int); 
 void ifsbreakup(char *, struct arglist *);
+void ifsfree(void);
 
 /* From arith.y */
 intmax_t arith(const char *);

index 5ab1648..c42a01c 100644
@@ -89,7 +89,7 @@ readcmd_handle_line(char *line, char **ap, size_t len)
 	
 	ifsbreakup(s, &arglist);
 	*arglist.lastp = NULL;
-	removerecordregions(0);
+	ifsfree();
 
 	for (sl = arglist.list; sl; sl = sl->next) {
 		/* remaining fields present, but no variables left. */

Cheers,

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

Hi Herbert,

I was experiencing weird symptoms with a local test script (for another program):

	$ sh -x t5523-push-upstream.sh -i -v
	[...]
	+ test_terminal git push -u upstream master
	+ die
	+ code=2
	+ test -n
	+ echo FATAL: Unexpected exit with code 2
	FATAL: Unexpected exit with code 2
	+ exit 1
	$ cat trash\ directory.t5523-push-upstream/err
	+ test_declared_prereq TTY
	+ return 1
	+ test_declared_prereq TTYREDIR
	+ return :T�    D�      \�      K�      l�      M�      1 �     D�      \�      K�      l�      M�      1 1
	return: 1: Illegal number: :T�  D�      \�      K�      l�      M�      1

The strange text there is supposed to be 127, I think, and it is not supposed to be redirected to err.

Bisects to f42e443bb. Reverting it avoids the problem. Any ideas before I investigate further?

Jonathan

Does this patch help?

index 2faaedd..5ace9ff 100644
@@ -1,3 +1,7 @@
+2010-10-18  Herbert Xu <[email protected]>
+
+	* Fix ifsfirst/ifslastp leak in casematch.
+
 2010-10-07  Herbert Xu <[email protected]>
 
 	* Fix EXEXEC status clobbering.

index d6c6416..1b77b7c 100644
@@ -1679,6 +1679,7 @@ casematch(union node *pattern, char *val)
 	STARTSTACKSTR(expdest);
 	argstr(pattern->narg.text, EXP_TILDE | EXP_CASE);
 	STACKSTRNUL(expdest);
+	ifsfree();
 	result = patmatch(stackblock(), val);
 	popstackmark(&smark);
 	return result;

Cheers,

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

Yes, thanks!

Hi Herbert et al,

Another puzzle bisecting to f42e443bb. This one comes from the grub-mkconfig script:

 $ sh -c 'datadir=/usr/share; pkgdatadir=${datadir}/`cat`' 2>&1 | cat -A
 cat: M-^\^M^F^HM-4^M^F^HM-(^M^F^H: No such file or directory$
 cat: M-(^M^F^H: No such file or directory$

Still reproducible with 016b529. I'll try to find time to look into it, but thought you might like to know nevertheless.

Regards,
Jonathan

Sure enough it was another leak, hopefully this is the last one :)

commit 49a94e2bab1e4f601a9fbdf9615d9e4e0150e412

Author: Herbert Xu <[email protected]>
Date:   Sun Nov 28 21:09:51 2010 +0800

[EXPAND] Free IFS state in evalbackcmd

On Sun, Nov 07, 2010 at 04:04:20PM -0600, Jonathan Nieder wrote:
>
> Herbert Xu wrote:
>
> > commit f42e443bb511ed3224f09b4fcf0772438ebdbbfa
> > Author: Herbert Xu <[email protected]>
> > Date: Wed Sep 8 20:07:26 2010 +0800
> >
> > [EXPAND] Fix ifsfirst/ifslastp leak
>
> Another puzzle bisecting to f42e443bb. This one comes from the > grub-mkconfig script:
>
> $ sh -c 'datadir=/usr/share; pkgdatadir=${datadir}/cat' 2>&1 | cat -A > cat: M-^\^M^F^HM-4^M^F^HM-(^M^F^H: No such file or directory$ > cat: M-(^M^F^H: No such file or directory$
>
> Still reproducible with 016b529. I'll try to find time to look into > it, but thought you might like to know nevertheless.

This is the symptom of another leak. In this case evalbackcmd occurs in the middle of an expansion (as it should) but the forked child never clears the previous IFS state.

This patch adds the missing ifsfree call.

This wasn't as much of a problem as the previously discovered leaks since all it means is that the child gets to carry around the parent's expansion state and the child is usually short-lived.

index 13572bf..d0a8304 100644
@@ -19,6 +19,7 @@
 
 	* Fixed trap/return regression due to SKIPEVAL removal.
 	* Allow the originator of EXERROR to set the exit status.
+	* Free IFS state in evalbackcmd.
 
 2010-10-18  Herbert Xu <[email protected]>
 

index 64aabb1..6e5c43e 100644
@@ -625,6 +625,7 @@ evalbackcmd(union node *n, struct backcmd *result)
 				dup2(pip[1], 1);
 				close(pip[1]);
 			}
+			ifsfree();
 			evaltreenr(n, EV_EXIT);
 			/* NOTREACHED */
 		}

Thanks,

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