Bug 79067 - [patch] /bin/sh should be more intelligent about IFS
Summary: [patch] /bin/sh should be more intelligent about IFS
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: standards (show other bugs)
Version: 5.3-STABLE
Hardware: Any Any
: Normal Affects Only Me
Assignee: Jilles Tjoelker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-03-21 01:20 UTC by Vsevolod Stakhov
Modified: 2023-09-24 05:51 UTC (History)
0 users

See Also:


Attachments
file.diff (3.05 KB, patch)
2005-03-21 01:20 UTC, Vsevolod Stakhov
no flags Details | Diff
file.diff (425 bytes, patch)
2005-03-21 01:20 UTC, Vsevolod Stakhov
no flags Details | Diff
sh-ifs-expand-netbsd.patch (4.20 KB, patch)
2009-04-04 12:06 UTC, Jilles Tjoelker
no flags Details | Diff
sh-ifs-expand.patch (861 bytes, patch)
2009-04-04 12:06 UTC, Jilles Tjoelker
no flags Details | Diff
moreifs.sh (1.82 KB, text/plain; charset=us-ascii)
2009-04-04 12:06 UTC, Jilles Tjoelker
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Stakhov 2005-03-21 01:20:06 UTC
/bin/sh in FreeBSD sometimes does not confirm POSIX. NetBSD people (David Laight)
has changed sh read built-in and expand.c. So I have adapted their changes to
FreeBSD. Now /bin/sh passes test from http://www.research.att.com/~gsf/public/ifs.sh
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2005-03-21 02:47:27 UTC
Responsible Changed
From-To: freebsd-bugs->freebsd-standards

Looks like a standards issue.
Comment 2 Craig Rodrigues freebsd_committer freebsd_triage 2006-05-16 05:45:49 UTC
Responsible Changed
From-To: freebsd-standards->stefanf

Over to Stefan, who has been fixing a lot of /bin/sh problems lately.
Comment 3 Jilles Tjoelker freebsd_committer freebsd_triage 2009-04-04 12:06:33 UTC
A patch similar to the 'read' part of this one was committed to
-CURRENT.

The other part of the patch in the PR is a bit different from NetBSD,
and is wrong: it drops a final empty argument in "$@". For example,
set -- a ''; set -- "$@"; echo $#  should give 2, but gives 1.

So I put in the NetBSD code, which does not have this issue
(sh-ifs-expand-netbsd.patch). Additionally, it cleans up the code a bit.

However, the NetBSD code has a related issue: it drops a final empty
argument in "$@"$s where $s contains one or more characters of IFS
whitespace.  For example,  s=' '; set -- a ''; set -- "$@"; echo $#
should give 2, but gives 1. A fix for this is in sh-ifs-expand.patch.

With the read change and both patches, the tests from
http://www.research.att.com/~gsf/public/ifs.sh all pass. Some more tests
with "$@" and IFS are in moreifs.sh.

-- 
Jilles Tjoelker
Comment 4 Jilles Tjoelker freebsd_committer freebsd_triage 2009-04-04 14:36:02 UTC
It seems the patches are a bit mangled, I've put them on a web server:

http://www.stack.nl/~jilles/unix/sh-ifs-expand-netbsd.patch
http://www.stack.nl/~jilles/unix/sh-ifs-expand.patch
http://www.stack.nl/~jilles/unix/moreifs.sh
Comment 5 dfilter service freebsd_committer freebsd_triage 2009-06-25 18:11:03 UTC
Author: jilles
Date: Thu Jun 25 17:10:51 2009
New Revision: 194975
URL: http://svn.freebsd.org/changeset/base/194975

Log:
  Improve IFS expansion using code from NetBSD.
  
  We now pass the ifs.sh testsuite.
  
  PR:		standards/79067
  Approved by:	ed (mentor) (implicit)
  Obtained from:	NetBSD

Modified:
  head/bin/sh/expand.c

Modified: head/bin/sh/expand.c
==============================================================================
--- head/bin/sh/expand.c	Thu Jun 25 16:48:13 2009	(r194974)
+++ head/bin/sh/expand.c	Thu Jun 25 17:10:51 2009	(r194975)
@@ -82,7 +82,7 @@ struct ifsregion {
 	struct ifsregion *next;	/* next region in list */
 	int begoff;		/* offset of start of region */
 	int endoff;		/* offset of end of region */
-	int nulonly;		/* search for nul bytes only */
+	int inquotes;		/* search for nul bytes only */
 };
 
 
@@ -936,13 +936,19 @@ numvar:
  */
 
 STATIC void
-recordregion(int start, int end, int nulonly)
+recordregion(int start, int end, int inquotes)
 {
 	struct ifsregion *ifsp;
 
 	if (ifslastp == NULL) {
 		ifsp = &ifsfirst;
 	} else {
+		if (ifslastp->endoff == start
+		    && ifslastp->inquotes == inquotes) {
+			/* extend previous area */
+			ifslastp->endoff = end;
+			return;
+		}
 		ifsp = (struct ifsregion *)ckmalloc(sizeof (struct ifsregion));
 		ifslastp->next = ifsp;
 	}
@@ -950,7 +956,7 @@ recordregion(int start, int end, int nul
 	ifslastp->next = NULL;
 	ifslastp->begoff = start;
 	ifslastp->endoff = end;
-	ifslastp->nulonly = nulonly;
+	ifslastp->inquotes = inquotes;
 }
 
 
@@ -969,75 +975,88 @@ ifsbreakup(char *string, struct arglist 
 	char *p;
 	char *q;
 	char *ifs;
-	int ifsspc;
-	int nulonly;
-
+	const char *ifsspc;
+	int had_param_ch = 0;
 
 	start = string;
-	ifsspc = 0;
-	nulonly = 0;
-	if (ifslastp != NULL) {
-		ifsp = &ifsfirst;
-		do {
-			p = string + ifsp->begoff;
-			nulonly = ifsp->nulonly;
-			ifs = nulonly ? nullstr :
-				( ifsset() ? ifsval() : " \t\n" );
-			ifsspc = 0;
-			while (p < string + ifsp->endoff) {
-				q = p;
-				if (*p == CTLESC)
+
+	if (ifslastp == NULL) {
+		/* Return entire argument, IFS doesn't apply to any of it */
+		sp = (struct strlist *)stalloc(sizeof *sp);
+		sp->text = start;
+		*arglist->lastp = sp;
+		arglist->lastp = &sp->next;
+		return;
+	}
+
+	ifs = ifsset() ? ifsval() : " \t\n";
+
+	for (ifsp = &ifsfirst; ifsp != NULL; ifsp = ifsp->next) {
+		p = string + ifsp->begoff;
+		while (p < string + ifsp->endoff) {
+			had_param_ch = 1;
+			q = p;
+			if (*p == CTLESC)
+				p++;
+			if (ifsp->inquotes) {
+				/* Only NULs (should be from "$@") end args */
+				if (*p != 0) {
 					p++;
-				if (strchr(ifs, *p)) {
-					if (!nulonly)
-						ifsspc = (strchr(" \t\n", *p) != NULL);
-					/* Ignore IFS whitespace at start */
-					if (q == start && ifsspc) {
-						p++;
-						start = p;
-						continue;
-					}
-					*q = '\0';
-					sp = (struct strlist *)stalloc(sizeof *sp);
-					sp->text = start;
-					*arglist->lastp = sp;
-					arglist->lastp = &sp->next;
+					continue;
+				}
+				ifsspc = NULL;
+			} else {
+				if (!strchr(ifs, *p)) {
 					p++;
-					if (!nulonly) {
-						for (;;) {
-							if (p >= string + ifsp->endoff) {
-								break;
-							}
-							q = p;
-							if (*p == CTLESC)
-								p++;
-							if (strchr(ifs, *p) == NULL ) {
-								p = q;
-								break;
-							} else if (strchr(" \t\n",*p) == NULL) {
-								if (ifsspc) {
-									p++;
-									ifsspc = 0;
-								} else {
-									p = q;
-									break;
-								}
-							} else
-								p++;
-						}
-					}
-					start = p;
-				} else
+					continue;
+				}
+				had_param_ch = 0;
+				ifsspc = strchr(" \t\n", *p);
+
+				/* Ignore IFS whitespace at start */
+				if (q == start && ifsspc != NULL) {
 					p++;
+					start = p;
+					continue;
+				}
 			}
-		} while ((ifsp = ifsp->next) != NULL);
-		if (*start || (!ifsspc && start > string)) {
+
+			/* Save this argument... */
+			*q = '\0';
 			sp = (struct strlist *)stalloc(sizeof *sp);
 			sp->text = start;
 			*arglist->lastp = sp;
 			arglist->lastp = &sp->next;
+			p++;
+
+			if (ifsspc != NULL) {
+				/* Ignore further trailing IFS whitespace */
+				for (; p < string + ifsp->endoff; p++) {
+					q = p;
+					if (*p == CTLESC)
+						p++;
+					if (strchr(ifs, *p) == NULL) {
+						p = q;
+						break;
+					}
+					if (strchr(" \t\n", *p) == NULL) {
+						p++;
+						break;
+					}
+				}
+			}
+			start = p;
 		}
-	} else {
+	}
+
+	/*
+	 * Save anything left as an argument.
+	 * Traditionally we have treated 'IFS=':'; set -- x$IFS' as
+	 * generating 2 arguments, the second of which is empty.
+	 * Some recent clarification of the Posix spec say that it
+	 * should only generate one....
+	 */
+	if (had_param_ch || *start != 0) {
 		sp = (struct strlist *)stalloc(sizeof *sp);
 		sp->text = start;
 		*arglist->lastp = sp;
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 6 dfilter service freebsd_committer freebsd_triage 2009-06-25 18:14:19 UTC
Author: jilles
Date: Thu Jun 25 17:14:06 2009
New Revision: 194977
URL: http://svn.freebsd.org/changeset/base/194977

Log:
  Fix some weirdnesses in the NetBSD IFS code,
  in particular "$@"$ifschar if the final positional parameter is empty.
  With the NetBSD code, adding the $ifschar removes a parameter.
  
  PR:		standards/79067
  Approved by:	ed (mentor) (implicit)

Modified:
  head/bin/sh/expand.c

Modified: head/bin/sh/expand.c
==============================================================================
--- head/bin/sh/expand.c	Thu Jun 25 17:11:27 2009	(r194976)
+++ head/bin/sh/expand.c	Thu Jun 25 17:14:06 2009	(r194977)
@@ -994,12 +994,12 @@ ifsbreakup(char *string, struct arglist 
 	for (ifsp = &ifsfirst; ifsp != NULL; ifsp = ifsp->next) {
 		p = string + ifsp->begoff;
 		while (p < string + ifsp->endoff) {
-			had_param_ch = 1;
 			q = p;
 			if (*p == CTLESC)
 				p++;
 			if (ifsp->inquotes) {
 				/* Only NULs (should be from "$@") end args */
+				had_param_ch = 1;
 				if (*p != 0) {
 					p++;
 					continue;
@@ -1007,10 +1007,10 @@ ifsbreakup(char *string, struct arglist 
 				ifsspc = NULL;
 			} else {
 				if (!strchr(ifs, *p)) {
+					had_param_ch = 1;
 					p++;
 					continue;
 				}
-				had_param_ch = 0;
 				ifsspc = strchr(" \t\n", *p);
 
 				/* Ignore IFS whitespace at start */
@@ -1019,6 +1019,7 @@ ifsbreakup(char *string, struct arglist 
 					start = p;
 					continue;
 				}
+				had_param_ch = 0;
 			}
 
 			/* Save this argument... */
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 7 dfilter service freebsd_committer freebsd_triage 2009-06-25 18:14:19 UTC
Author: jilles
Date: Thu Jun 25 17:14:06 2009
New Revision: 194977
URL: http://svn.freebsd.org/changeset/base/194977

Log:
  Fix some weirdnesses in the NetBSD IFS code,
  in particular "$@"$ifschar if the final positional parameter is empty.
  With the NetBSD code, adding the $ifschar removes a parameter.
  
  PR:		standards/79067
  Approved by:	ed (mentor) (implicit)

Modified:
  head/bin/sh/expand.c

Modified: head/bin/sh/expand.c
==============================================================================
--- head/bin/sh/expand.c	Thu Jun 25 17:11:27 2009	(r194976)
+++ head/bin/sh/expand.c	Thu Jun 25 17:14:06 2009	(r194977)
@@ -994,12 +994,12 @@ ifsbreakup(char *string, struct arglist 
 	for (ifsp = &ifsfirst; ifsp != NULL; ifsp = ifsp->next) {
 		p = string + ifsp->begoff;
 		while (p < string + ifsp->endoff) {
-			had_param_ch = 1;
 			q = p;
 			if (*p == CTLESC)
 				p++;
 			if (ifsp->inquotes) {
 				/* Only NULs (should be from "$@") end args */
+				had_param_ch = 1;
 				if (*p != 0) {
 					p++;
 					continue;
@@ -1007,10 +1007,10 @@ ifsbreakup(char *string, struct arglist 
 				ifsspc = NULL;
 			} else {
 				if (!strchr(ifs, *p)) {
+					had_param_ch = 1;
 					p++;
 					continue;
 				}
-				had_param_ch = 0;
 				ifsspc = strchr(" \t\n", *p);
 
 				/* Ignore IFS whitespace at start */
@@ -1019,6 +1019,7 @@ ifsbreakup(char *string, struct arglist 
 					start = p;
 					continue;
 				}
+				had_param_ch = 0;
 			}
 
 			/* Save this argument... */
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 8 Jilles Tjoelker freebsd_committer freebsd_triage 2010-05-15 16:55:55 UTC
State Changed
From-To: open->closed

Fixed in 8.x/9.x, no MFC to 7.x planned. 


Comment 9 Jilles Tjoelker freebsd_committer freebsd_triage 2010-05-15 16:55:55 UTC
Responsible Changed
From-To: stefanf->jilles

I did the work.