[Partial patch] IFS and read builtin

[Partial patch] IFS and read builtin

[Harald van Dijk]

[-- Attachment #1: Type: text/plain, Size: 1493 bytes --]

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.)

[-- Attachment #2: dash-read-ifs.patch --]
[-- Type: text/plain, Size: 2138 bytes --]

diff --git a/src/expand.c b/src/expand.c
index f2f964c..3ba1a38 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -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;
diff --git a/src/expand.h b/src/expand.h
index 405af0b..8eb5f07 100644
--- a/src/expand.h
+++ b/src/expand.h
@@ -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 *);
diff --git a/src/miscbltin.c b/src/miscbltin.c
index 5ab1648..898ab09 100644
--- a/src/miscbltin.c
+++ b/src/miscbltin.c
@@ -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);
 

Re: [Partial patch] IFS and read builtin

[Jilles Tjoelker]

On Mon, Aug 23, 2010 at 12:20:12AM +0200, Harald van Dijk wrote:
> 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.

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

> 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.

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

Re: [Partial patch] IFS and read builtin

[Harald van Dijk]

On 23/08/10 01:00, Jilles Tjoelker wrote:
> On Mon, Aug 23, 2010 at 12:20:12AM +0200, Harald van Dijk wrote:
>>[...]
>>   echo a:b | { IFS=: read a b; echo $a; }
>>[...]
> 
> This has already been fixed in a totally different way in master. See
> git commits near 95a60b2936e8835963bfb08eadc0edf9dddf0498.

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?

>>    $ 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||
>>[...]
> 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.

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.

> 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

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.

Re: [Partial patch] IFS and read builtin

[Jilles Tjoelker]

On Mon, Aug 23, 2010 at 02:03:01AM +0200, Harald van Dijk wrote:
> On 23/08/10 01:00, Jilles Tjoelker wrote:
> > On Mon, Aug 23, 2010 at 12:20:12AM +0200, Harald van Dijk wrote:
> >>[...]
> >>   echo a:b | { IFS=: read a b; echo $a; }
> >>[...]
> > 
> > This has already been fixed in a totally different way in master. See
> > git commits near 95a60b2936e8835963bfb08eadc0edf9dddf0498.

> 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?

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.

> >>    $ 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||
> >>[...]
> > 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.

> 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.

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

> > 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

> 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 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

Re: [Partial patch] IFS and read builtin

[Harald van Dijk]

On 23/08/10 21:35, Jilles Tjoelker wrote:
> 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.

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.

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

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=", "]
> 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.

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.

> 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.

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

Re: [Partial patch] IFS and read builtin

[Jilles Tjoelker]

On Tue, Aug 24, 2010 at 12:51:47AM +0200, Harald van Dijk wrote:
> On 23/08/10 21:35, Jilles Tjoelker wrote:
> > 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.

> 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.

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.

> [IFS=", "]
> > 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.

> 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.

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.

> > 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.

> 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.

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

Re: [Partial patch] IFS and read builtin

[Harald van Dijk]

On 25/08/10 00:51, Jilles Tjoelker wrote:
> 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.

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.

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

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

Re: [Partial patch] IFS and read builtin

[Herbert Xu]

Jilles Tjoelker <jilles@stack.nl> wrote:
>
> 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,,").

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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [Partial patch] IFS and read builtin

[Herbert Xu]

Harald van Dijk <harald@gigawatt.nl> wrote:
> [-- text/plain, encoding 7bit, charset: ISO-8859-1, 20 lines --]
> 
> 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.

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

> 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.

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 <herbert@gondor.apana.org.au>
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.
    
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/ChangeLog b/ChangeLog
index 3c26149..a51975c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2010-09-08  Herbert Xu <herbert@gondor.apana.org.au>
+
+	* Fix ifsfirst/ifslastp leak.
+
 2010-09-08  maximilian attems <max@stro.at>
 
 	* Debug compile fix.
diff --git a/src/expand.c b/src/expand.c
index f2f964c..d6c6416 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -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
diff --git a/src/expand.h b/src/expand.h
index 405af0b..4251a4a 100644
--- a/src/expand.h
+++ b/src/expand.h
@@ -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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [Partial patch] IFS and read builtin

[Herbert Xu]

On Wed, Sep 08, 2010 at 07:53:24PM +0800, Herbert Xu wrote:
> 
> commit 5c7042771753d5a968b2b7263cf9f4e02fa3820e
> Author: Herbert Xu <herbert@gondor.apana.org.au>
> Date:   Wed Sep 8 19:51:10 2010 +0800
> 
>     [EXPAND] Fix ifsfirst/ifslastp leak

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

commit f42e443bb511ed3224f09b4fcf0772438ebdbbfa
Author: Herbert Xu <herbert@gondor.apana.org.au>
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.
    
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/ChangeLog b/ChangeLog
index 3c26149..a51975c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2010-09-08  Herbert Xu <herbert@gondor.apana.org.au>
+
+	* Fix ifsfirst/ifslastp leak.
+
 2010-09-08  maximilian attems <max@stro.at>
 
 	* Debug compile fix.
diff --git a/src/expand.c b/src/expand.c
index f2f964c..d6c6416 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -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
diff --git a/src/expand.h b/src/expand.h
index 405af0b..4251a4a 100644
--- a/src/expand.h
+++ b/src/expand.h
@@ -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 *);
diff --git a/src/miscbltin.c b/src/miscbltin.c
index 5ab1648..c42a01c 100644
--- a/src/miscbltin.c
+++ b/src/miscbltin.c
@@ -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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [Partial patch] IFS and read builtin

[Jonathan Nieder]

Hi Herbert,

Herbert Xu wrote:

> commit f42e443bb511ed3224f09b4fcf0772438ebdbbfa
> Author: Herbert Xu <herbert@gondor.apana.org.au>
> 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.

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

Re: [Partial patch] IFS and read builtin

[Herbert Xu]

On Sat, Oct 16, 2010 at 02:15:48PM -0500, Jonathan Nieder wrote:
> 
> Bisects to f42e443bb.  Reverting it avoids the problem.  Any ideas
> before I investigate further?

Does this patch help?

diff --git a/ChangeLog b/ChangeLog
index 2faaedd..5ace9ff 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2010-10-18  Herbert Xu <herbert@gondor.apana.org.au>
+
+	* Fix ifsfirst/ifslastp leak in casematch.
+
 2010-10-07  Herbert Xu <herbert@gondor.apana.org.au>
 
 	* Fix EXEXEC status clobbering.
diff --git a/src/expand.c b/src/expand.c
index d6c6416..1b77b7c 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [Partial patch] IFS and read builtin

[Jonathan Nieder]

Herbert Xu wrote:
> On Sat, Oct 16, 2010 at 02:15:48PM -0500, Jonathan Nieder wrote:

>> Bisects to f42e443bb.  Reverting it avoids the problem.  Any ideas
>> before I investigate further?
>
> Does this patch help?

Yes, thanks!

Re: [Partial patch] IFS and read builtin

[Jonathan Nieder]

Hi Herbert et al,

Herbert Xu wrote:

> commit f42e443bb511ed3224f09b4fcf0772438ebdbbfa
> Author: Herbert Xu <herbert@gondor.apana.org.au>
> 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.

Regards,
Jonathan

Re: [Partial patch] IFS and read builtin

[Herbert Xu]

On Sun, Nov 07, 2010 at 04:04:20PM -0600, Jonathan Nieder wrote:
>
> 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.

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

commit 49a94e2bab1e4f601a9fbdf9615d9e4e0150e412
Author: Herbert Xu <herbert@gondor.apana.org.au>
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 <herbert@gondor.apana.org.au>
    > > 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.
    
    Reported-by: Jonathan Nieder <jrnieder@gmail.com>
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/ChangeLog b/ChangeLog
index 13572bf..d0a8304 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -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 <herbert@gondor.apana.org.au>
 
diff --git a/src/eval.c b/src/eval.c
index 64aabb1..6e5c43e 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[Partial patch] IFS and read builtin

[Harald van Dijk]

[-- Attachment #1: Type: text/plain, Size: 1469 bytes --]

  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

[-- Attachment #2: dash-read-ifs.patch --]
[-- Type: text/plain, Size: 2137 bytes --]

diff --git a/src/expand.c b/src/expand.c
index f2f964c..3ba1a38 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -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;
diff --git a/src/expand.h b/src/expand.h
index 405af0b..8eb5f07 100644
--- a/src/expand.h
+++ b/src/expand.h
@@ -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 *);
diff --git a/src/miscbltin.c b/src/miscbltin.c
index 5ab1648..898ab09 100644
--- a/src/miscbltin.c
+++ b/src/miscbltin.c
@@ -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);