Test failure with negative substring offsets

Test failure with negative substring offsets

[Peter Stephenson]

This is now failing:

./D04parameter.ztst: starting.
*** /tmp/zsh.ztst.err.18663	Mon May 23 15:58:07 2011
--- /tmp/zsh.ztst.terr.18663	Mon May 23 15:58:07 2011
***************
*** 1 ****
! (eval):2: substring expression: 3 < 5
--- 1 ----
! (eval):2: substring expression: 3 < 0
Test ./D04parameter.ztst failed: error output differs from expected as shown above for:
   foo="123456789"
   print ${foo:5:-6}
Was testing: Regression test for total length < 0 in string
./D04parameter.ztst: test failed.
**************************************
0 successful test scripts, 1 failure, 0 skipped
**************************************
make: *** [check] Error 1

I'll wait for it to stop failing.  I'm guessing this is to do with
29224.

I think possibly the error could be clearer.  It's not really clear what 3
being less than 5 has got to do with the problem, i.e. that counting
6 back from the end puts you before the start offset.

-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: Test failure with negative substring offsets

[Mikael Magnusson]

On 23 May 2011 17:10, Peter Stephenson <Peter.Stephenson@csr.com> wrote:
> This is now failing:
>
> ./D04parameter.ztst: starting.
> *** /tmp/zsh.ztst.err.18663     Mon May 23 15:58:07 2011
> --- /tmp/zsh.ztst.terr.18663    Mon May 23 15:58:07 2011
> ***************
> *** 1 ****
> ! (eval):2: substring expression: 3 < 5
> --- 1 ----
> ! (eval):2: substring expression: 3 < 0
> Test ./D04parameter.ztst failed: error output differs from expected as shown above for:
>   foo="123456789"
>   print ${foo:5:-6}
> Was testing: Regression test for total length < 0 in string
> ./D04parameter.ztst: test failed.
> **************************************
> 0 successful test scripts, 1 failure, 0 skipped
> **************************************
> make: *** [check] Error 1
>
> I'll wait for it to stop failing.  I'm guessing this is to do with
> 29224.

Odd,
./D04parameter.ztst: starting.
./D04parameter.ztst: all tests successful.
**************************************
1 successful test script, 0 failures, 0 skipped
**************************************

> I think possibly the error could be clearer.  It's not really clear what 3
> being less than 5 has got to do with the problem, i.e. that counting
> 6 back from the end puts you before the start offset.

bash does this,
bash: -6: substring expression < 0
I guess we could be more explicit and say something like "substring
expression error: end offset before start offset" or something?

-- 
Mikael Magnusson

Re: Test failure with negative substring offsets

[İsmail Dönmez]

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

hi;

On Mon, May 23, 2011 at 5:24 PM, Mikael Magnusson <mikachu@gmail.com> wrote:

> On 23 May 2011 17:10, Peter Stephenson <Peter.Stephenson@csr.com> wrote:
> > This is now failing:
> >
> > ./D04parameter.ztst: starting.
> > *** /tmp/zsh.ztst.err.18663     Mon May 23 15:58:07 2011
> > --- /tmp/zsh.ztst.terr.18663    Mon May 23 15:58:07 2011
> > ***************
> > *** 1 ****
> > ! (eval):2: substring expression: 3 < 5
> > --- 1 ----
> > ! (eval):2: substring expression: 3 < 0
> > Test ./D04parameter.ztst failed: error output differs from expected as
> shown above for:
> >   foo="123456789"
> >   print ${foo:5:-6}
> > Was testing: Regression test for total length < 0 in string
> > ./D04parameter.ztst: test failed.
> > **************************************
> > 0 successful test scripts, 1 failure, 0 skipped
> > **************************************
> > make: *** [check] Error 1
> >
> > I'll wait for it to stop failing.  I'm guessing this is to do with
> > 29224.
>
> Odd,
> ./D04parameter.ztst: starting.
> ./D04parameter.ztst: all tests successful.
> **************************************
> 1 successful test script, 0 failures, 0 skipped
> **************************************
>

It passes for me on 64bit machines and fails on 32bit machines. Hope that
helps.

Regards,
ismail

Re: Test failure with negative substring offsets

[Mikael Magnusson]

Does this make it work? (and if so, why? :)

---
 Src/subst.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Src/subst.c b/Src/subst.c
index 5628c11..e4cd061 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -2888,9 +2888,9 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int ssub)
                    if (offset_hack_argzero)
                        alen++;
                    if (length_set) {
-                       if (length < 0)
+                       if (length < (zlong)0)
                            length += alen - offset;
-                       if (length < 0) {
+                       if (length < (zlong)0) {
                            zerr("substring expression: %d < %d",
                                 length + offset, offset);
                            return NULL;
@@ -2927,20 +2927,20 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int ssub)
                    }
                    given_offset = offset;
                    MB_METACHARINIT();
-                   if (length_set && length < 0)
+                   if (length_set && length < (zlong)0)
                        length -= offset;
                    for (sptr = val; *sptr && offset; ) {
                        sptr += MB_METACHARLEN(sptr);
                        offset--;
                    }
                    if (length_set) {
-                       if (length < 0) {
+                       if (length < (zlong)0) {
                            MB_METACHARINIT();
                            for (eptr = val; *eptr; ) {
                                eptr += MB_METACHARLEN(eptr);
                                length++;
                            }
-                           if (length < 0) {
+                           if (length < (zlong)0) {
                                zerr("substring expression: %d < %d",
                                     length + given_offset, given_offset);
                                return NULL;

--

Re: Test failure with negative substring offsets

[Bart Schaefer]

On May 23,  5:52pm, Mikael Magnusson wrote:
}
} Does this make it work? (and if so, why? :)

No.

Re: Test failure with negative substring offsets

[Mikael Magnusson]

On 23 May 2011 17:58, Bart Schaefer <schaefer@brasslantern.com> wrote:
> On May 23,  5:52pm, Mikael Magnusson wrote:
> }
> } Does this make it work? (and if so, why? :)
>
> No.

Okay, good, it was a long shot. I saw in my original diff that I had
removed an if (length < (zlong)0) and left the cast out in my new
code.

-- 
Mikael Magnusson

Re: Test failure with negative substring offsets

[Peter Stephenson]

On Mon, 23 May 2011 17:41:50 +0200
İsmail Dönmez <ismail@namtrac.org> wrote:
> It passes for me on 64bit machines and fails on 32bit machines. Hope
> that helps.

ahhhhh... yes, that was exactly the clue I needed, thank you.

stdarg doesn't know the arguments need to be converted to integers to
fit the size being claimed in the printf prototype.

There might be a few more of these around, hidden by the fact that
there's only one integer argument and it's being passed little-endian,
so it would fail e.g. on Solaris 32-bit with large file support.

The second part is paranoia...  the stdarg manual says the va_list ap is
undefined after return from a function, so it's safest to run va_end()
on it within the function where it's been used.

Index: Src/subst.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/subst.c,v
retrieving revision 1.123
diff -p -u -r1.123 subst.c
--- Src/subst.c	19 May 2011 16:24:38 -0000	1.123
+++ Src/subst.c	23 May 2011 15:53:08 -0000
@@ -2892,7 +2892,7 @@ paramsubst(LinkList l, LinkNode n, char 
 			    length += alen - offset;
 			if (length < 0) {
 			    zerr("substring expression: %d < %d",
-			         length + offset, offset);
+			         (int)(length + offset), (int)offset);
 			    return NULL;
 			}
 		    } else
@@ -2942,7 +2942,8 @@ paramsubst(LinkList l, LinkNode n, char 
 			    }
 			    if (length < 0) {
 				zerr("substring expression: %d < %d",
-				     length + given_offset, given_offset);
+				     (int)(length + given_offset),
+				     (int)given_offset);
 				return NULL;
 			    }
 			}
Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.258
diff -p -u -r1.258 utils.c
--- Src/utils.c	9 May 2011 09:49:09 -0000	1.258
+++ Src/utils.c	23 May 2011 15:53:09 -0000
@@ -155,7 +155,6 @@ VA_DCL
     VA_START(ap, fmt);
     VA_GET_ARG(ap, fmt, const char *);
     zwarning(NULL, fmt, ap);
-    va_end(ap);
     errflag = 1;
 }
 
@@ -175,7 +174,6 @@ VA_DCL
     VA_GET_ARG(ap, cmd, const char *);
     VA_GET_ARG(ap, fmt, const char *);
     zwarning(cmd, fmt, ap);
-    va_end(ap);
     errflag = 1;
 }
 
@@ -193,7 +191,6 @@ VA_DCL
     VA_START(ap, fmt);
     VA_GET_ARG(ap, fmt, const char *);
     zwarning(NULL, fmt, ap);
-    va_end(ap);
 }
 
 /**/
@@ -212,7 +209,6 @@ VA_DCL
     VA_GET_ARG(ap, cmd, const char *);
     VA_GET_ARG(ap, fmt, const char *);
     zwarning(cmd, fmt, ap);
-    va_end(ap);
 }
 
 
@@ -236,7 +232,6 @@ VA_DCL
 	fclose(file);
     } else
 	zerrmsg(stderr, message, ap);
-    va_end(ap);
 }
 
 #endif /* DEBUG */
@@ -341,6 +336,8 @@ zerrmsg(FILE *file, const char *fmt, va_
 	}
     putc('\n', file);
     fflush(file);
+
+    va_end(ap);
 }
 
 /* Output a single character, for the termcap routines.     *

-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: Test failure with negative substring offsets

[Peter Stephenson]

On Mon, 23 May 2011 16:56:14 +0100
Peter Stephenson <Peter.Stephenson@csr.com> wrote:
> The second part is paranoia...  the stdarg manual says the va_list ap
> is undefined after return from a function, so it's safest to run
> va_end() on it within the function where it's been used.

gag... but it also says

       Each invocation of va_start() must be matched by a corresponding  invo‐
       cation of va_end() in the same function.

so we're caught between the devil and the deep blue sea.  (I believe the
"devil" was the wide outermost seam on a ship, which took a long time to
put tar into, hence "having the devil to pay", which is also
appropriate to zsh releases.)

The implication might be that ap might point to any argument but the
va_end()could be doing weird things with the current function context.

I'll leave this one out since no one's ever seen a problem.

-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: Test failure with negative substring offsets

[Bart Schaefer]

On May 23,  4:56pm, Peter Stephenson wrote:
}
} stdarg doesn't know the arguments need to be converted to integers to
} fit the size being claimed in the printf prototype.

Er, wouldn't it therefore have been better to change the prototype to
expect a long?

That doesn't affect the rest of the paranoia.
 

Re: Test failure with negative substring offsets

[Peter Stephenson]

On Mon, 23 May 2011 09:10:52 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On May 23,  4:56pm, Peter Stephenson wrote:
> }
> } stdarg doesn't know the arguments need to be converted to integers
> to } fit the size being claimed in the printf prototype.
> 
> Er, wouldn't it therefore have been better to change the prototype to
> expect a long?

It's not necessarily a long, so it would still need a cast.  It would
certainly have been possible to use %ld / (long)..., however.

-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: Test failure with negative substring offsets

[Mikael Magnusson]

On 23 May 2011 18:10, Bart Schaefer <schaefer@brasslantern.com> wrote:
> On May 23,  4:56pm, Peter Stephenson wrote:
> }
> } stdarg doesn't know the arguments need to be converted to integers to
> } fit the size being claimed in the printf prototype.

This is a much better conclusion than mine.

> Er, wouldn't it therefore have been better to change the prototype to
> expect a long?

It doesn't expect a long, it expects a zlong, which can be a long or a
long long.

#ifdef ZSH_64_BIT_TYPE
typedef ZSH_64_BIT_TYPE zlong;
#ifdef ZSH_64_BIT_UTYPE
typedef ZSH_64_BIT_UTYPE zulong;
#else
typedef unsigned zlong zulong;
#endif
#else
typedef long zlong;
typedef unsigned long zulong;
#endif

But why does this code use zlong at all? 2^31 characters should be
long enough for everyone. (And things will probably fall over for
other reasons before that point, no?) But maybe the autoconf macro
could define a ZSH_PRINTF_SPECIFIER to %ld or %lld depending on what
zlong is?

-- 
Mikael Magnusson

Re: Test failure with negative substring offsets

[Peter Stephenson]

On Mon, 23 May 2011 18:19:04 +0200
Mikael Magnusson <mikachu@gmail.com> wrote:
> But maybe the autoconf macro
> could define a ZSH_PRINTF_SPECIFIER to %ld or %lld depending on what
> zlong is?

That would be useful, unfortunately those aren't the only cases...

     if test "$zsh_cv_64_bit_type" = no; then
       zsh_64_BIT_TYPE(quad_t, zsh_cv_64_bit_type)
     fi
     if test "$zsh_cv_64_bit_type" = no; then
       zsh_64_BIT_TYPE(__int64_t, zsh_cv_64_bit_type)
     fi

I'd settle for just casting to long here.

-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK




Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: Test failure with negative substring offsets

[Mikael Magnusson]

On 23 May 2011 18:27, Peter Stephenson <Peter.Stephenson@csr.com> wrote:
> On Mon, 23 May 2011 18:19:04 +0200
> Mikael Magnusson <mikachu@gmail.com> wrote:
>> But maybe the autoconf macro
>> could define a ZSH_PRINTF_SPECIFIER to %ld or %lld depending on what
>> zlong is?
>
> That would be useful, unfortunately those aren't the only cases...
>
>     if test "$zsh_cv_64_bit_type" = no; then
>       zsh_64_BIT_TYPE(quad_t, zsh_cv_64_bit_type)
>     fi
>     if test "$zsh_cv_64_bit_type" = no; then
>       zsh_64_BIT_TYPE(__int64_t, zsh_cv_64_bit_type)
>     fi
>
> I'd settle for just casting to long here.

Where do you mean? %d and (long)foo shouldn't work, it would need to
be %ld. Or you mean in those two cases? Then we would need a
ZSH_POSSIBLY_CAST_WEIRD_TYPE_TO_LONG define too :).

-- 
Mikael Magnusson

Re: Test failure with negative substring offsets

[Peter Stephenson]

On Mon, 23 May 2011 18:34:03 +0200
Mikael Magnusson <mikachu@gmail.com> wrote:
> > I'd settle for just casting to long here.
> 
> Where do you mean?

I mean what I said before...

> It would certainly have been possible to use %ld / (long)..., however.

-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: Test failure with negative substring offsets

[Peter Stephenson]

On Mon, 23 May 2011 17:40:33 +0100
Peter Stephenson <Peter.Stephenson@csr.com> wrote:
> > It would certainly have been possible to use %ld / (long)...,
> > however.

... except to prolong this discussion absurdly, zerrmsg() doesn't
actually handle full printf specifications anyway, and %l means
something completely different (which will no doubt be documented in the
production version but seems to be do with a string with a length).

I think I'll go and make some releases instead.

-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK




Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom