Re: [PATCH] fix shell bug in ${var%pattern} expansion

2010-10-13 Thread Jilles Tjoelker
On Mon, Oct 11, 2010 at 07:19:14PM -0700, David O'Brien wrote:
 At $WORK we hit a bug where ${var%/*} was not producing the correct
 expansion.  There are two patches to fix this.  I prefer the first
 as I feel it is cleaner from an API perspective.  I've also added
 a regression testcase that shows the problem.

 Thoughts?

Thank you, I think this is the mysterious bug I encountered a while ago
while making changes to arithmetic expansion, heavily using the stack
string. It only failed when /etc/rc.d/nfsserver was called from /etc/rc
on boot.

Your patch looks good (the first one) and works for me. The second patch
indeed seems a hack rather than fixing the problem properly.

Style bug:
 +growstrstackblock(int n) {
The opening brace should be on its own line.

Your test is too fragile: it often fails to detect the bug. Calling like
  sh -c '. expansion/trim4.0'
gives the correct output even with a buggy sh. I propose something like
this, or perhaps with an additional string comparison:

# $FreeBSD$

v1=/homes/SOME_USER
v2=
v3=C123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789

while [ ${#v2} -lt 2000 ]; do
v4=${v2} ${v1%/*} $v3
if [ ${#v4} -ne $((${#v2} + ${#v3} + 8)) ]; then
echo bad: ${#v4} -ne $((${#v2} + ${#v3} + 8))
fi
v2=x$v2
v3=y$v3
done

-- 
Jilles Tjoelker
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org


Re: [PATCH] fix shell bug in ${var%pattern} expansion

2010-10-13 Thread David O'Brien
On Wed, Oct 13, 2010 at 11:42:48PM +0200, Jilles Tjoelker wrote:
 Style bug:
  +growstrstackblock(int n) {
 The opening brace should be on its own line.

Indeed.  I'm surprised I did that.  Thank you for catching it.

 Your test is too fragile: it often fails to detect the bug. Calling like
   sh -c '. expansion/trim4.0'
 gives the correct output even with a buggy sh. I propose something like
 this, or perhaps with an additional string comparison:

I also like this test better.

Thank you for the review.

-- 
-- David  (obr...@freebsd.org)
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org


[PATCH] fix shell bug in ${var%pattern} expansion

2010-10-11 Thread David O'Brien
At $WORK we hit a bug where ${var%/*} was not producing the correct
expansion.  There are two patches to fix this.  I prefer the first
as I feel it is cleaner from an API perspective.  I've also added
a regression testcase that shows the problem.

Thoughts?

-- 
-- David  (obr...@freebsd.org)


Commit log:
Do not assume in growstackstr() that a precious character will be
immediately written into the stack after the call.  Instead let the
caller manage the space left.
Currently growstackstr()'s assumption causes problems with
STACKSTRNUL() where we want to be able to turn a stack into a C
string, and later pretend the NUL is not there.

This fixes a bug in STACKSTRNUL() (that grew the stack) where:
1. STADJUST() called after a STACKSTRNUL() results in an improper
   adjust.  This can be seen in ${var%pattern} and ${var%%pattern}
   evaluation.
2. Memory leak in STPUTC() called after a STACKSTRNUL().

--%--%--%--%--%--
 
Index: bin/sh/memalloc.h
===
--- bin/sh/memalloc.h   (revision 213714)
+++ bin/sh/memalloc.h   (working copy)
@@ -67,9 +67,16 @@ void ungrabstackstr(char *, char *);
 #define stackblock() stacknxt
 #define stackblocksize() stacknleft
 #define STARTSTACKSTR(p)   p = stackblock(), sstrnleft = stackblocksize()
-#define STPUTC(c, p)   (--sstrnleft = 0? (*p++ = (c)) : (p = growstackstr(), 
*p++ = (c)))
+#define STPUTC(c, p)   (--sstrnleft = 0? (*p++ = (c)) : (p = growstackstr(), 
--sstrnleft, *p++ = (c)))
 #define CHECKSTRSPACE(n, p){ if (sstrnleft  n) p = makestrspace(); }
 #define USTPUTC(c, p)  (--sstrnleft, *p++ = (c))
+/*
+ * STACKSTRNUL's use is where we want to be able to turn a stack
+ * (non-sentinel, character counting string) into a C string,
+ * and later pretend the NUL is not there.
+ * Note: Because of STACKSTRNUL's semantics, STACKSTRNUL cannot be used
+ * on a stack that will grabstackstr()ed.
+ */
 #define STACKSTRNUL(p) (sstrnleft == 0? (p = growstackstr(), *p = '\0') : (*p 
= '\0'))
 #define STUNPUTC(p)(++sstrnleft, --p)
 #define STTOPC(p)  p[-1]

Index: bin/sh/histedit.c
===
--- bin/sh/histedit.c   (revision 213714)
+++ bin/sh/histedit.c   (working copy)
@@ -418,7 +418,7 @@ fc_replace(const char *s, char *p, char 
} else
STPUTC(*s++, dest);
}
-   STACKSTRNUL(dest);
+   STPUTC('\0', dest);
dest = grabstackstr(dest);
 
return (dest);

Index: bin/sh/memalloc.c
===
--- bin/sh/memalloc.c   (revision 213714)
+++ bin/sh/memalloc.c   (working copy)
@@ -295,6 +295,12 @@ grabstackblock(int len)
  * is space for at least one character.
  */
 
+static char *
+growstrstackblock(int n) {
+   growstackblock();
+   sstrnleft = stackblocksize() - n;
+   return stackblock() + n;
+}
 
 char *
 growstackstr(void)
@@ -304,12 +310,10 @@ growstackstr(void)
len = stackblocksize();
if (herefd = 0  len = 1024) {
xwrite(herefd, stackblock(), len);
-   sstrnleft = len - 1;
+   sstrnleft = len;
return stackblock();
}
-   growstackblock();
-   sstrnleft = stackblocksize() - len - 1;
-   return stackblock() + len;
+   return growstrstackblock(len);
 }
 
 
@@ -323,9 +327,7 @@ makestrspace(void)
int len;
 
len = stackblocksize() - sstrnleft;
-   growstackblock();
-   sstrnleft = stackblocksize() - len;
-   return stackblock() + len;
+   return growstrstackblock(len);
 }
 
 
--%--%--%--%--%--
ALTERNATE PATCH:

Index: bin/sh-1/memalloc.h
===
--- bin/sh-1/memalloc.h (revision 213696)
+++ bin/sh-1/memalloc.h (working copy)
@@ -70,7 +70,17 @@ void ungrabstackstr(char *, char *);
 #define STPUTC(c, p)   (--sstrnleft = 0? (*p++ = (c)) : (p = growstackstr(), 
*p++ = (c)))
 #define CHECKSTRSPACE(n, p){ if (sstrnleft  n) p = makestrspace(); }
 #define USTPUTC(c, p)  (--sstrnleft, *p++ = (c))
-#define STACKSTRNUL(p) (sstrnleft == 0? (p = growstackstr(), *p = '\0') : (*p 
= '\0'))
+/*
+ * growstackstr() has the assumption that a precious character will be
+ * immediately written into it, so it sets up 'sstrnleft' appropriately.
+ * But that is not the case for STACKSTRNUL, where we want to be able to
+ * turn a stack (non-sentinel, character counting string) into a C string,
+ * and later pretend the NUL is not there (without adjusting 'sstrnleft').
+ * Note: because of STACKSTRNUL's semantics, STACKSTRNUL cannot be used on
+ * a stack that will grabstackstr()ed.
+ */
+#define STACKSTRNUL(p) (sstrnleft == 0? (p = growstackstr(), *p = '\0', 
++sstrnleft) : (*p = '\0'))
+//#define STACKSTRNUL(p)