Two issues found with -fsanitize=undefined

Two issues found with -fsanitize=undefined

[Markus Trippelsdorf]

I've build zsh trunk with -fsanitize=undefined and two issue popped up
while running the testsuite:

1)
 pattern.c:2645:12: runtime error: signed integer overflow: 1234567890123456789 * 10 cannot be represented in type 'long int'
Test ./D02glob.ztst failed: error output differs from expected as shown above for:

2)
 compmatch.c:341:6: runtime error: null pointer passed as argument 2, which is declared to never be null}
Test ./Y02compmatch.ztst failed: output differs from expected as shown above for:

-- 
Markus

Re: Two issues found with -fsanitize=undefined

[Bart Schaefer]

On Sep 17,  9:57am, Markus Trippelsdorf wrote:
}
} I've build zsh trunk with -fsanitize=undefined and two issue popped up
} while running the testsuite:

It would be helpful if you included at least part of the output (after the
"for:") to indicate WHICH tests reported these errors.

} 1)
}  pattern.c:2645:12: runtime error: signed integer overflow: 1234567890123456789 * 10 cannot be represented in type 'long int'

That one is fairly easy to recognize (the string 1234567890123456789 is
in the test case itself) but this one:

}  compmatch.c:341:6: runtime error: null pointer passed as argument 2, which is declared to never be null}
} Test ./Y02compmatch.ztst failed: output differs from expected as shown above for:

Could be anything.  The error indicates that "matchbuf" is NULL at the
point where we expect to copy it into a new buffer, but if there is a
deeper reason why that is unexpectedly NULL it would be better to fix
that than to simply skip the copy.

Re: Two issues found with -fsanitize=undefined

[Markus Trippelsdorf]

On 2015.09.17 at 10:16 -0700, Bart Schaefer wrote:
> On Sep 17,  9:57am, Markus Trippelsdorf wrote:
> }
> } I've build zsh trunk with -fsanitize=undefined and two issue popped up
> } while running the testsuite:
> 
> It would be helpful if you included at least part of the output (after the
> "for:") to indicate WHICH tests reported these errors.
> 
> }  compmatch.c:341:6: runtime error: null pointer passed as argument 2, which is declared to never be null}
> } Test ./Y02compmatch.ztst failed: output differs from expected as shown above for:
> 
> Could be anything.  The error indicates that "matchbuf" is NULL at the
> point where we expect to copy it into a new buffer, but if there is a
> deeper reason why that is unexpectedly NULL it would be better to fix
> that than to simply skip the copy.

Sorry:

./Y02compmatch.ztst: starting.
*** /tmp/zsh.ztst.out.27917     Thu Sep 17 19:20:46 2015
--- /tmp/zsh.ztst.tout.27917    Thu Sep 17 19:20:46 2015
***************
*** 1,3 ****
  line: {tst nolistbeep }{}
! COMPADD:{}
  INSERT_POSITIONS:{14}
--- 1,3 ----
  line: {tst nolistbeep }{}
! COMPADD:{compmatch.c:341:6: runtime error: null pointer passed as argument 2, which is declared to never be null}
  INSERT_POSITIONS:{14}
Test ./Y02compmatch.ztst failed: output differs from expected as shown above for:
  example1_list=(
        kshoptionprint        shglob              
        listambiguous         shinstdin           
        listbeep              shnullcmd           
        listpacked            shoptionletters     
        listrowsfirst         shortloops          
        listtypes             shwordsplit
   )
 options_matcher='L:|[nN][oO]= M:_= M:{A-Z}={a-z}'
 test_code $options_matcher example1_list
 comptest $'tst nolistbee\t'
Was testing: Documentation example for options, input "nolistbee"
./Y02compmatch.ztst: test failed.

And it is quite easy to reproduce (with gcc-5):

zsh-code % ./.preconfig
zsh-code % CC="gcc -fsanitize=undefined" ./configure 
zsh-code % make && make check 

BTW why isn't the make -j option passed on?
e.g. "make -j4" still builds single threaded.

-- 
Markus

Re: Two issues found with -fsanitize=undefined

[Bart Schaefer]

On Sep 17,  7:24pm, Markus Trippelsdorf wrote:
}
} BTW why isn't the make -j option passed on?
} e.g. "make -j4" still builds single threaded.

The zsh build depends on rebuilding some of its own Makefiles on the
fly.  This means there's no safe way to parallelize some significant
parts of it, and it's easier to just discard those options than to
work out where it's safe to apply them.

Re: Two issues found with -fsanitize=undefined

[Peter Stephenson]

On Thu, 17 Sep 2015 09:57:59 +0200
Markus Trippelsdorf <markus@trippelsdorf.de> wrote:
> 1)
>  pattern.c:2645:12: runtime error: signed integer overflow: 1234567890123456789 * 10 cannot be represented in type 'long int'
> Test ./D02glob.ztst failed: error output differs from expected as shown above for:

This might shut the compiler up, although the existing code should do
the right thing anyway.  I don't have gcc 5.

pws

diff --git a/Src/pattern.c b/Src/pattern.c
index 3b55ccf..af56bd9 100644
--- a/Src/pattern.c
+++ b/Src/pattern.c
@@ -220,8 +220,10 @@ typedef union upat *Upat;
 #if defined(ZSH_64_BIT_TYPE) || defined(LONG_IS_64_BIT)
 typedef zlong zrange_t;
 #define ZRANGE_T_IS_SIGNED	(1)
+#define ZRANGE_MAX ZLONG_MAX
 #else
 typedef unsigned long zrange_t;
+#define ZRANGE_MAX ULONG_MAX
 #endif
 
 #ifdef MULTIBYTE_SUPPORT
@@ -2641,19 +2643,30 @@ patmatch(Upat prog)
 	    start = compend = patinput;
 	    comp = 0;
 	    while (patinput < patinend && idigit(*patinput)) {
-		if (comp)
-		    comp *= 10;
-		comp += *patinput - '0';
+		int out_of_range = 0;
+		int digit = *patinput - '0';
+		if (comp > ZRANGE_MAX / (zlong)10) {
+		    out_of_range = 1;
+		} else {
+		    zrange_t c10 = comp ? comp * 10 : 0;
+		    if (ZRANGE_MAX - c10 < digit) {
+			out_of_range = 1;
+		    } else {
+			comp = c10;
+			comp += digit;
+		    }
+		}
 		patinput++;
 		compend++;
 
-		if (comp & ((zrange_t)1 << (sizeof(comp)*8 -
+		if (out_of_range ||
+		    (comp & ((zrange_t)1 << (sizeof(comp)*8 -
 #ifdef ZRANGE_T_IS_SIGNED
 					    2
 #else
 					    1
 #endif
-				))) {
+				)))) {
 		    /*
 		     * Out of range (allowing for signedness, which
 		     * we need if we are using zlongs).
diff --git a/Src/zsh.h b/Src/zsh.h
index 4e2cb65..9c7e5d9 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -36,6 +36,12 @@
  */
 #ifdef ZSH_64_BIT_TYPE
 typedef ZSH_64_BIT_TYPE zlong;
+#if defind(ZLONG_IS_LONG_LONG) && defined(LLONG_MAX)
+#define ZLONG_MAX LLONG_MAX
+#else
+/* umm... */
+#define  ZLONG_MAX ((zlong)9223372036854775807)
+#endif
 #ifdef ZSH_64_BIT_UTYPE
 typedef ZSH_64_BIT_UTYPE zulong;
 #else
@@ -44,6 +50,7 @@ typedef unsigned zlong zulong;
 #else
 typedef long zlong;
 typedef unsigned long zulong;
+#define ZLONG_MAX LONG_MAX
 #endif
 
 /*

Re: Two issues found with -fsanitize=undefined

[Markus Trippelsdorf]

On 2015.09.19 at 20:57 +0100, Peter Stephenson wrote:
> On Thu, 17 Sep 2015 09:57:59 +0200
> Markus Trippelsdorf <markus@trippelsdorf.de> wrote:
> > 1)
> >  pattern.c:2645:12: runtime error: signed integer overflow: 1234567890123456789 * 10 cannot be represented in type 'long int'
> > Test ./D02glob.ztst failed: error output differs from expected as shown above for:
> 
> This might shut the compiler up, although the existing code should do
> the right thing anyway.  I don't have gcc 5.

Thanks. It fixes one issue. And please note that all issues reported by
running the instrumented shell, build with -fsanitize=undefined, are
_real_ undefined behavior bugs. So this is not about shutting the
compiler up, but making sure that undefined behavior isn't invoked
anymore.

BTW clang's -fsanitize=undefined report these additional cases during
D02glob.ztst:

./D02glob.ztst: starting.
exec.c:2240:6: runtime error: index 8 out of bounds for type 'int [8]'
exec.c:2048:10: runtime error: index 8 out of bounds for type 'int [8]'
exec.c:2122:7: runtime error: index 8 out of bounds for type 'int [8]'

-- 
Markus

Re: Two issues found with -fsanitize=undefined

[Peter Stephenson]

On Sat, 19 Sep 2015 22:18:14 +0200
Markus Trippelsdorf <markus@trippelsdorf.de> wrote:
> ./D02glob.ztst: starting.
> exec.c:2240:6: runtime error: index 8 out of bounds for type 'int [8]'
> exec.c:2048:10: runtime error: index 8 out of bounds for type 'int [8]'
> exec.c:2122:7: runtime error: index 8 out of bounds for type 'int [8]'
 
MULTIOUNIT is 8. struct multio is defined as

struct multio {
    int ct;		/* # of redirections on this fd                 */
    int rflag;		/* 0 if open for reading, 1 if open for writing */
    int pipe;		/* fd of pipe if ct > 1                         */
    int fds[MULTIOUNIT]; /* list of src/dests redirected to/from this fd */
};

so something is probably up here when ct is 8 and the structure needs
reallocating.

	if (mfds[fd1]->ct == 1) {	/* split the stream */
           /* ... */
	} else {		/* add another fd to an already split stream */
	    int fdN;
	    if(!(mfds[fd1]->ct % MULTIOUNIT)) {
		int new = sizeof(struct multio) + sizeof(int) * mfds[fd1]->ct;
		int old = new - sizeof(int) * MULTIOUNIT;
		mfds[fd1] = hrealloc((char *)mfds[fd1], old, new);
	    }
	    if ((fdN = movefd(fd2)) < 0) {
		zerr("multio failed for fd %d: %e", fd2, errno);
		closemnodes(mfds);
		return;
	    }
	    mfds[fd1]->fds[mfds[fd1]->ct++] = fdN;
	}

However, it looks right.  You end up with MULTIOUNIT + ct fd's
available, which is what you want: ct goes from 8 to 9 with index 8
being used out of 0 to 15 allocated.  The zerr() before the return means
it doesn't matter if we don't actually increment ct as the structure is
never used (memory is on the heap).

It may be the compiler isn't actually looking at the memory allocated,
only the definition of the structure.  Certainly valgrind has never
complained here and this is something it should pick up.

pws

Re: Two issues found with -fsanitize=undefined

[Bart Schaefer]

On Sep 19, 11:35pm, Peter Stephenson wrote:
} Subject: Re: Two issues found with -fsanitize=undefined
}
} On Sat, 19 Sep 2015 22:18:14 +0200
} Markus Trippelsdorf <markus@trippelsdorf.de> wrote:
} > ./D02glob.ztst: starting.
} > exec.c:2240:6: runtime error: index 8 out of bounds for type 'int [8]'
} > exec.c:2048:10: runtime error: index 8 out of bounds for type 'int [8]'
} > exec.c:2122:7: runtime error: index 8 out of bounds for type 'int [8]'
}  
} It may be the compiler isn't actually looking at the memory allocated,
} only the definition of the structure.  Certainly valgrind has never
} complained here and this is something it should pick up.

Yep, this is clearly a case of the compiled-in bounds-checking making
the assumption that the size of the allocated space is the same as
the size of the declared structure.  We're making deliberate use of
C's lack of strict pointer bounds.

We can shut it up with a "#pragma clang ..." but it's going to get
annoying to do that for every possible compiler.

Re: Two issues found with -fsanitize=undefined

[Peter Stephenson]

On Sat, 19 Sep 2015 20:57:51 +0100
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> --- a/Src/zsh.h
> +++ b/Src/zsh.h
> @@ -36,6 +36,12 @@
>   */
>  #ifdef ZSH_64_BIT_TYPE
>  typedef ZSH_64_BIT_TYPE zlong;
> +#if defind(ZLONG_IS_LONG_LONG) && defined(LLONG_MAX)

I've fixed this, which broke compilation on 32-bit systems with 64-bit
long long.

Here's a patch to neaten up the case (removing a magic number) where
zlong is a 64-bit long.

pws

diff --git a/Src/zsh.h b/Src/zsh.h
index b98fc5c..dd05961 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -39,9 +39,13 @@ typedef ZSH_64_BIT_TYPE zlong;
 #if defined(ZLONG_IS_LONG_LONG) && defined(LLONG_MAX)
 #define ZLONG_MAX LLONG_MAX
 #else
+#ifdef ZLONG_IS_LONG_64
+#define ZLONG_MAX LONG_MAX
+#else
 /* umm... */
 #define  ZLONG_MAX ((zlong)9223372036854775807)
 #endif
+#endif
 #ifdef ZSH_64_BIT_UTYPE
 typedef ZSH_64_BIT_UTYPE zulong;
 #else
diff --git a/configure.ac b/configure.ac
index d7db8ba..c3bd713 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1079,9 +1079,15 @@ main() { return sizeof(ino_t) < 8; }
 fi
 AH_TEMPLATE([ZLONG_IS_LONG_LONG],
 [Define to 1 if the zlong type uses long long int.])
+AH_TEMPLATE([ZLONG_IS_LONG_64],
+[Define to 1 if the zlong type uses 64-bit long int.])
 if test "$zsh_cv_64_bit_type" = "long long"; then
   dnl Remember this so we can get (s)printf output right.
   AC_DEFINE(ZLONG_IS_LONG_LONG)
+else
+  if test "$zsh_cv_64_bit_type" = "long"; then
+      AC_DEFINE(ZLONG_IS_LONG_64)
+  fi
 fi
 
 dnl We'll blithely assume (f)printf supports the same types as sprintf.

Re: Two issues found with -fsanitize=undefined

[Peter Stephenson]

On Thu, 17 Sep 2015 09:57:59 +0200
Markus Trippelsdorf <markus@trippelsdorf.de> wrote:
> 2)
>  compmatch.c:341:6: runtime error: null pointer passed as argument 2, which is declared to never be null}
> Test ./Y02compmatch.ztst failed: output differs from expected as shown above for:

This might be benign, if the final argument is also zero; this should
tell us.

pws

diff --git a/Src/Zle/compmatch.c b/Src/Zle/compmatch.c
index 05ae43a..b5728a5 100644
--- a/Src/Zle/compmatch.c
+++ b/Src/Zle/compmatch.c
@@ -338,8 +338,15 @@ add_match_str(Cmatcher m, char *l, char *w, int wl, int sfx)
 	    char *buf;
 
 	    buf = (char *) zalloc(blen);
-	    memcpy(buf, matchbuf, matchbuflen);
-	    zfree(matchbuf, matchbuflen);
+	    if (matchbuf) {
+		memcpy(buf, matchbuf, matchbuflen);
+		zfree(matchbuf, matchbuflen);
+	    }
+#ifdef DEBUG
+	    else {
+		DPUTS(matchbuflen, "matchbuflen with no matchbuf");
+	    }
+#endif
 	    matchbuf = buf;
 	    matchbuflen = blen;
 	}

Re: Two issues found with -fsanitize=undefined

[Markus Trippelsdorf]

On 2015.09.21 at 20:13 +0100, Peter Stephenson wrote:
> On Thu, 17 Sep 2015 09:57:59 +0200
> Markus Trippelsdorf <markus@trippelsdorf.de> wrote:
> > 2)
> >  compmatch.c:341:6: runtime error: null pointer passed as argument 2, which is declared to never be null}
> > Test ./Y02compmatch.ztst failed: output differs from expected as shown above for:
> 
> This might be benign, if the final argument is also zero; this should
> tell us.

Yes:
matchbuflen: (null) with no matchbuf: 0

It looks like the issue doesn't happen with a -DDEBUG build.
I've changed the DPUTS line to:
 fprintf(stderr, "matchbuflen: %s with no matchbuf: %x", matchbuf, matchbuflen);

-- 
Markus

Re: Two issues found with -fsanitize=undefined

[Peter Stephenson]

On Mon, 21 Sep 2015 22:04:13 +0200
Markus Trippelsdorf <markus@trippelsdorf.de> wrote:
> Yes:
> matchbuflen: (null) with no matchbuf: 0
> It looks like the issue doesn't happen with a -DDEBUG build.
> I've changed the DPUTS line to:
>  fprintf(stderr, "matchbuflen: %s with no matchbuf: %x", matchbuf, matchbuflen);

That's fine.  If matchbuf is NULL, matchbuflen *should* be 0.

I presume you're missing that

  DPUTS(matchbuflen, "matchbuflen with no matchbuf");

is (roughly) equivalent to

  if (matchbuflen)
     fprintf(stderr, "matchbuflen with no matcbuf");

pws

Re: Two issues found with -fsanitize=undefined

[Markus Trippelsdorf]

On 2015.09.21 at 21:21 +0100, Peter Stephenson wrote:
> On Mon, 21 Sep 2015 22:04:13 +0200
> Markus Trippelsdorf <markus@trippelsdorf.de> wrote:
> > Yes:
> > matchbuflen: (null) with no matchbuf: 0
> > It looks like the issue doesn't happen with a -DDEBUG build.
> > I've changed the DPUTS line to:
> >  fprintf(stderr, "matchbuflen: %s with no matchbuf: %x", matchbuf, matchbuflen);
> 
> That's fine.  If matchbuf is NULL, matchbuflen *should* be 0.
> 
> I presume you're missing that
> 
>   DPUTS(matchbuflen, "matchbuflen with no matchbuf");
> 
> is (roughly) equivalent to
> 
>   if (matchbuflen)
>      fprintf(stderr, "matchbuflen with no matcbuf");

Well, only if DEBUG is defined. And as I wrote above the issue doesn't
happen at all for -DDEBUG.

-- 
Markus

Re: Two issues found with -fsanitize=undefined

[Bart Schaefer]

On Mon, Sep 21, 2015 at 1:34 PM, Markus Trippelsdorf
<markus@trippelsdorf.de> wrote:
>
>
> Well, only if DEBUG is defined. And as I wrote above the issue doesn't
> happen at all for -DDEBUG.

You mean that even without Peter's latest patch(es), the -fsanitize
checking does not report that a null pointer is being passed in the
case where -DDEBUG is defined?

That must mean (something along the lines of) defining DEBUG is
somehow changing the behavior of malloc(0) so it returns a pointer to
a zero-sized buffer rather than returning a null pointer.

Re: Two issues found with -fsanitize=undefined

[Markus Trippelsdorf]

On 2015.09.21 at 13:56 -0700, Bart Schaefer wrote:
> On Mon, Sep 21, 2015 at 1:34 PM, Markus Trippelsdorf
> <markus@trippelsdorf.de> wrote:
> >
> >
> > Well, only if DEBUG is defined. And as I wrote above the issue doesn't
> > happen at all for -DDEBUG.
> 
> You mean that even without Peter's latest patch(es), the -fsanitize
> checking does not report that a null pointer is being passed in the
> case where -DDEBUG is defined?

No sorry. It was a local setup issue. Please ignore. 
Peter's patch is fine.

-- 
Markus