Valgrind is complaining unitialized values in freea (malloca.c:135)

[Marc Nieper-Wißkirchen]

In freea in malloca.c, a possibly uninitialized indicator word is used for
a comparison so that Valgrind reports: "Conditional jump or move depends on
uninitialised value(s)".

Valgrind is not smart enough to understand the logic in freea.

It would be nice if the warning could be silenced, either by amending freea
slightly (it seems that a similar thing has already been done for Clang
warnings) or by reporting the issue to the Valgrind developers so that they
can special-case gnulib's freea.

--

Marc

[Tim Rühsen]

Post by Marc Nieper-Wißkirchen
In freea in malloca.c, a possibly uninitialized indicator word is used for
a comparison so that Valgrind reports: "Conditional jump or move depends on
uninitialised value(s)".
Valgrind is not smart enough to understand the logic in freea.
It would be nice if the warning could be silenced, either by amending freea
slightly (it seems that a similar thing has already been done for Clang
warnings) or by reporting the issue to the Valgrind developers so that they
can special-case gnulib's freea.

I also see several false positives from clang's Undefined Sanitizer due to
alloca 'magic' (reallocations on stack space ?). This might not be directly
related, but I think there is a common coding pattern.

glob.c:1738:23: runtime error: index 64 out of bounds for type 'char *[64]'
#0 0x557545 in glob_in_dir /home/tim/src/wget2/lib/glob.c:1738:40
#1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior glob.c:1738:23 in
glob.c:1739:27: runtime error: index 64 out of bounds for type 'char *[64]'
#0 0x5575d4 in glob_in_dir /home/tim/src/wget2/lib/glob.c:1739:27
#1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior glob.c:1739:27 in
glob.c:1811:21: runtime error: index 64 out of bounds for type 'char *[64]'
#0 0x55845e in glob_in_dir /home/tim/src/wget2/lib/glob.c:1811:21
#1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16

Regards, Tim

[Marc Nieper-Wißkirchen]

I just noticed the file lib/malloca.valgrind, which can be used with the
Valgrind option suppressions.

Marc

Post by Tim Rühsen

Post by Marc Nieper-Wißkirchen
In freea in malloca.c, a possibly uninitialized indicator word is used

for

Post by Marc Nieper-Wißkirchen
a comparison so that Valgrind reports: "Conditional jump or move depends

on

Post by Marc Nieper-Wißkirchen
uninitialised value(s)".
Valgrind is not smart enough to understand the logic in freea.
It would be nice if the warning could be silenced, either by amending

freea

Post by Marc Nieper-Wißkirchen
slightly (it seems that a similar thing has already been done for Clang
warnings) or by reporting the issue to the Valgrind developers so that

they

Post by Marc Nieper-Wißkirchen
can special-case gnulib's freea.

I also see several false positives from clang's Undefined Sanitizer due to
alloca 'magic' (reallocations on stack space ?). This might not be directly
related, but I think there is a common coding pattern.
glob.c:1738:23: runtime error: index 64 out of bounds for type 'char *[64]'
#0 0x557545 in glob_in_dir /home/tim/src/wget2/lib/glob.c:1738:40
#1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior glob.c:1738:23 in
glob.c:1739:27: runtime error: index 64 out of bounds for type 'char *[64]'
#0 0x5575d4 in glob_in_dir /home/tim/src/wget2/lib/glob.c:1739:27
#1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior glob.c:1739:27 in
glob.c:1811:21: runtime error: index 64 out of bounds for type 'char *[64]'
#0 0x55845e in glob_in_dir /home/tim/src/wget2/lib/glob.c:1811:21
#1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16
Regards, Tim

[Bruno Haible]

[Changing the subject, as this is an unrelated topic.]

Hi Tim,

Post by Tim Rühsen
I also see several false positives from clang's Undefined Sanitizer due to
alloca 'magic' (reallocations on stack space ?). This might not be directly
related, but I think there is a common coding pattern.
glob.c:1738:23: runtime error: index 64 out of bounds for type 'char *[64]'
#0 0x557545 in glob_in_dir /home/tim/src/wget2/lib/glob.c:1738:40
#1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior glob.c:1738:23 in
glob.c:1739:27: runtime error: index 64 out of bounds for type 'char *[64]'
#0 0x5575d4 in glob_in_dir /home/tim/src/wget2/lib/glob.c:1739:27
#1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior glob.c:1739:27 in
glob.c:1811:21: runtime error: index 64 out of bounds for type 'char *[64]'
#0 0x55845e in glob_in_dir /home/tim/src/wget2/lib/glob.c:1811:21
#1 0x54ded1 in rpl_glob /home/tim/src/wget2/lib/glob.c:1306:16

It obviously does not handle the 'struct globnames' allocated with
the FLEXSIZEOF macro (lines 1719..1732).

Bruno

[Paul Eggert]

Post by Bruno Haible
It obviously does not handle the 'struct globnames' allocated with
the FLEXSIZEOF macro (lines 1719..1732).

Yes, and I installed the attached patch into Gnulib to try to fix this.
I hope it is enough to pacify clang's Undefined Sanitizer. If not, Tim,
please let us know, and thanks for reporting the problem.

I'll CC: this to Adhemerval Zanella, who's working on porting Gnulib
glob.c back to glibc, as I worry that this patch (or something like it)
should also be ported back.

[Adhemerval Zanella]

Post by Bruno Haible
It obviously does not handle the 'struct globnames' allocated with
the FLEXSIZEOF macro (lines 1719..1732).

Yes, and I installed the attached patch into Gnulib to try to fix this. I hope it is enough to pacify clang's Undefined Sanitizer. If not, Tim, please let us know, and thanks for reporting the problem.
I'll CC: this to Adhemerval Zanella, who's working on porting Gnulib glob.c back to glibc, as I worry that this patch (or something like it) should also be ported back.

Thanks for heads up Paul, I intend to send the sync patch for gnulib
glob today. In fact I decided to *not* sync flexmember because with
following patch I intend to send (which are in the original thread)
make flexmember unnecessary.

[Paul Eggert]

Post by Adhemerval Zanella
In fact I decided to *not* sync flexmember because with
following patch I intend to send (which are in the original thread)
make flexmember unnecessary.

I see that you sent these proposed patches to glibc glob in the thread
starting here:

https://sourceware.org/ml/libc-alpha/2017-08/msg01079.html

and I am looking into merging that into Gnulib glob. However, I don't
see why the patch makes flexmember unnecessary. Even with that patch,
there is still a datatype 'struct globnames' that has a fixed-size
member array 'names', and the code still indexes the 'names' component
past its bounds. That is, the recently-fixed problem is not
out-of-bounds access into a local variable; it is out-of-bounds access
into either malloc- or alloca-allocated storage, via a pointer to a type
that has fixed-size bounds; the C standard does not allow this. So as
far as I can see, a fix is still necessary even with your patch.

I'll try to resolve this and come up with a patch to Gnulib, and also
with a patch to follow on to your proposed glibc patch. There are
several other details that need to be looked at.

[Adhemerval Zanella]

Post by Paul Eggert

Post by Adhemerval Zanella
In fact I decided to *not* sync flexmember because with
following patch I intend to send (which are in the original thread)
make flexmember unnecessary.

https://sourceware.org/ml/libc-alpha/2017-08/msg01079.html
and I am looking into merging that into Gnulib glob. However, I don't see why the patch makes flexmember unnecessary. Even with that patch, there is still a datatype 'struct globnames' that has a fixed-size member array 'names', and the code still indexes the 'names' component past its bounds. That is, the recently-fixed problem is not out-of-bounds access into a local variable; it is out-of-bounds access into either malloc- or alloca-allocated storage, via a pointer to a type that has fixed-size bounds; the C standard does not allow this. So as far as I can see, a fix is still necessary even with your patch.
I'll try to resolve this and come up with a patch to Gnulib, and also with a patch to follow on to your proposed glibc patch. There are several other details that need to be looked at.

The patchset I sent today only have patches to sync with gnulib
implementation, cleanup some non required support for glibc, and
consolidate glibc various implementation to simplify the code.
It is not meant to fix any current bugs or issues (aside the ones
from gnulib sync), but rather to refactor the code.

The one I am referring is in my previous interaction that I
want to re-send in following day (maybe after this set being
pushed upstream) and it basically rewrites how the 'struct
globnames' are defined and accessed [1] to use glibc 'dynarray'
internal data structure. That's why I said flexmember is not
really required.

[1] https://sourceware.org/ml/libc-alpha/2017-08/msg00453.html

[Paul Eggert]

Post by Adhemerval Zanella
The patchset I sent today only have patches to sync with gnulib
implementation, cleanup some non required support for glibc, and
consolidate glibc various implementation to simplify the code.
It is not meant to fix any current bugs or issues (aside the ones
from gnulib sync), but rather to refactor the code.

OK, thanks. With that in mind I attempted to merge that patchset into
Gnulib by installing the attached. In the process I found and fixed a
few minor problems with the patchset. I'll follow up on libc-alpha, by
proposing a patch there that merges the latest Gnulib into your patchset.

Post by Adhemerval Zanella
The one I am referring is in my previous interaction that I
want to re-send in following day (maybe after this set being
pushed upstream) and it basically rewrites how the 'struct
globnames' are defined and accessed [1] to use glibc 'dynarray'
internal data structure. That's why I said flexmember is not
really required.
[1]https://sourceware.org/ml/libc-alpha/2017-08/msg00453.html

OK, I plan to look at this later.

It's good to get glibc and Gnulib merged first (as I'm attempting to
help do with the attached) before redoing glob.c storage allocation.
Since the flexible-member stuff is needed to keep Gnulib glob.c working
on some platforms, the attached patch keeps that stuff in. We can remove
it later when we make the dynarray changes to Gnulib, assuming dynarray
doesn't need it.

[Bruno Haible]

Hi Benno,

Post by Paul Eggert
With that in mind I attempted to merge that patchset into
Gnulib by installing the attached.

This part of the patch:

@@ -190,11 +175,8 @@ convert_dirent (const struct dirent *source)
struct readdir_result result = { NULL, };
return result;
}
- else
- {
- struct readdir_result result = READDIR_RESULT_INITIALIZER (source);
- return result;
- }
+ struct readdir_result result = READDIR_RESULT_INITIALIZER (source);
+ return result;
}

#ifndef COMPILE_GLOB64
@@ -208,31 +190,11 @@ convert_dirent64 (const struct dirent64 *source)
struct readdir_result result = { NULL, };
return result;
}
- else
- {
- struct readdir_result result = READDIR_RESULT_INITIALIZER (source);
- return result;
- }
+ struct readdir_result result = READDIR_RESULT_INITIALIZER (source);
+ return result;
}
#endif


discards your change
http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=e29357c8f445320cb3084352579fb2648cce4d0f

I haven't gotten any answer to my inquiries
https://lists.gnu.org/archive/html/bug-gnulib/2017-07/msg00009.html

Can you confirm that you can use a newer GCC on Haiku, and that therefore said
change was not actually needed?

Bruno

[Benno Schulenberg]

Hi Bruno,

Post by Bruno Haible
[...]
discards your change
http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=e29357c8f445320cb3084352579fb2648cce4d0f
I haven't gotten any answer to my inquiries
https://lists.gnu.org/archive/html/bug-gnulib/2017-07/msg00009.html
Can you confirm that you can use a newer GCC on Haiku, and that therefore said
change was not actually needed?

I will look into that over the weekend. Thanks for caring for this.

Benno

[Benno Schulenberg]

Post by Bruno Haible
[...]
discards your change
http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=e29357c8f445320cb3084352579fb2648cce4d0f
I haven't gotten any answer to my inquiries
https://lists.gnu.org/archive/html/bug-gnulib/2017-07/msg00009.html
Can you confirm that you can use a newer GCC on Haiku, and that therefore said
change was not actually needed?

I couldn't find it on the site of Haiku, but finally found on the ffmpeg
site that one can get a newer gcc (5.4.0 at the moment) on Haiku by making
sure that "/system/bin/x86" is at the beginning of PATH:

https://trac.ffmpeg.org/wiki/CompilationGuide/Haiku

With that gcc, nano can be compiled on Haiku without a problem, also with
current gnulib. So the change of the above patch is not needed for those
who know where a less ancient toolchain hides itself on a Haiku system.

Thanks,

Benno

[Bruno Haible]

Hi Benno,

Post by Benno Schulenberg

Post by Bruno Haible
Can you confirm that you can use a newer GCC on Haiku, and that therefore said
change was not actually needed?

I couldn't find it on the site of Haiku, but finally found on the ffmpeg
site that one can get a newer gcc (5.4.0 at the moment) on Haiku by making
https://trac.ffmpeg.org/wiki/CompilationGuide/Haiku
With that gcc, nano can be compiled on Haiku without a problem, also with
current gnulib. So the change of the above patch is not needed for those
who know where a less ancient toolchain hides itself on a Haiku system.

Thanks for the confirmation. So we're only losing IRIX cc, not Haiku, by
assuming C99 or newer.

Bruno

[Bruno Haible]

Hi Paul,

This change

@@ -1780,8 +1627,7 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
while (1)
{
struct globnames *old = names;
- size_t i;
- for (i = 0; i < cur; ++i)
+ for (size_t i = 0; i < cur; ++i)
free (names->name[i]);
names = names->next;
/* NB: we will not leak memory here if we exit without
@@ -1806,8 +1652,7 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
while (1)
{
struct globnames *old = names;
- size_t i;
- for (i = 0; i < cur; ++i)
+ for (size_t i = 0; i < cur; ++i)
new_gl_pathv[pglob->gl_offs + pglob->gl_pathc++]
= names->name[i];
names = names->next;

causes a build failure of a testdir of module 'glob' on NetBSD 7 and OpenBSD 6.

On NetBSD 7.0:

gcc -DHAVE_CONFIG_H -I. -I../../gllib -I.. -DGNULIB_STRICT_CHECKING=1 -I/home/bruno/include -Wall -g -O2 -MT glob.o -MD -MP -MF .deps/glob.Tpo -c -o glob.o ../../gllib/glob.c
../../gllib/glob.c: In function 'glob_in_dir':
../../gllib/glob.c:1630:15: error: 'for' loop initial declarations are only allowed in C99 mode
for (size_t i = 0; i < cur; ++i)
^
../../gllib/glob.c:1630:15: note: use option -std=c99 or -std=gnu99 to compile your code
../../gllib/glob.c:1655:15: error: 'for' loop initial declarations are only allowed in C99 mode
for (size_t i = 0; i < cur; ++i)
^
*** Error code 1

On OpenBSD 6.0:

gcc -DHAVE_CONFIG_H -I. -I../../gllib -I.. -DGNULIB_STRICT_CHECKING=1 -I/home/bruno/include -Wall -g -O2 -MT glob.o -MD -MP -MF .deps/glob.Tpo -c -o glob.o ../../gllib/glob.c
../../gllib/glob.c: In function 'glob_in_dir':
../../gllib/glob.c:1630: error: 'for' loop initial declaration used outside C99 mode
../../gllib/glob.c:1655: error: 'for' loop initial declaration used outside C99 mode
*** Error 1 in gllib (Makefile:1201 'glob.o')

The following patch fixes it.


2017-08-24 Bruno Haible <***@clisp.org>

glob: Fix compilation error on NetBSD 7.0 and OpenBSD 6.0.
* modules/glob (Depends-on): Add c99.

diff --git a/modules/glob b/modules/glob
index 0d64124..48cf387 100644
--- a/modules/glob
+++ b/modules/glob
@@ -12,6 +12,7 @@ lib/globfree.c
m4/glob.m4

Depends-on:
+c99
extensions
largefile
snippet/c++defs

[Bruno Haible]

On IRIX 6.5 with cc:

Even with the c99 dependency, the compiler produces this error:

cc -32 -DHAVE_CONFIG_H -I. -I../../gllib -I.. -DGNULIB_STRICT_CHECKING=1 -I/u/guest/bruno/prefix-32-cc/include -g -c -o glob.o ../../gllib/glob.c
cfe: Warning 728: ./stddef.h, line 104: Long double not supported; double assumed.
long double __ld ;
--^
cfe: Error: ../../gllib/glob_internal.h, line 24: Syntax Error
{
^

This is a problem with 'static inline'. Adding AC_REQUIRE([AC_C_INLINE])
helps on this one, but the compilation fails a little later:

cc -32 -DHAVE_CONFIG_H -I. -I../../gllib -I.. -DGNULIB_STRICT_CHECKING=1 -I/u/guest/bruno/prefix-32-cc/include -g -c -o glob.o ../../gllib/glob.c
cfe: Warning 728: ./stddef.h, line 104: Long double not supported; double assumed.
long double __ld ;
--^
cfe: Error: ../../gllib/glob.c, line 178: Syntax Error
struct readdir_result result = { source->d_name, (source)->d_ino == 0, } ;
--^
cfe: Error: ../../gllib/glob.c, line 565: Syntax Error
signed char drive_root = 0 ;
------^
cfe: Error: ../../gllib/glob.c, line 1630: Syntax Error
for (size_t i = 0; i < cur; ++i)
-------------------^

It doesn't grok
- the READDIR_RESULT_INITIALIZER, i.e. brace initializer syntax,
- declaration-after-statement syntax,
- 'for' with variable declaration.

Should we bury the support for IRIX cc? (There is also a gcc on the machine
I have access to.)

Bruno

[Paul Eggert]

Post by Bruno Haible
Should we bury the support for IRIX cc?

Yes, let's. SGI stopped supporting IRIX in December 2013, and there is little
point to our continuing to support it.

[Tom G. Christensen]

<snip>

Because it's not in c99 mode.

You need MIPSpro 7.4.0 or later for c99 support and it requires the
'-c99' argument when invoking cc or invoking the compiler as c99.

-tgc

[Bruno Haible]

Hi Paul,

With the newest glob, a testdir of the 'glob' module fails to compile on
native Windows (mingw and MSVC), with the same link error.

../gllib/libgnu.a(glob.o): In function `rpl_glob':
/home/bruno/testdir-glob/build-mingw32/gllib/../../gllib/glob.c:538: undefined reference to `rpl_glob_pattern_p'

Since I don't have time to analyze it right now, I'm attaching the relevant
files from the build directory.

Other than that, the testdir builds and passes its tests on all platforms
I tested - except for IRIX -: FreeBSD, NetBSD, OpenBSD, AIX, HP-UX, Solaris,
Mac OS X, Cygwin.

Bruno

[Paul Eggert]

Post by Bruno Haible
/home/bruno/testdir-glob/build-mingw32/gllib/../../gllib/glob.c:538: undefined reference to `rpl_glob_pattern_p'

Thanks, I don't have easy access to MS-Windows, but looking at the symptoms I
installed the attached patch into Gnulib. I hope this fixes it.

[Bruno Haible]

Post by Paul Eggert
looking at the symptoms I
installed the attached patch into Gnulib. I hope this fixes it.

Thanks, I confirm the patch fixes it (on both mingw and msvc).

Bruno

[Tim Rühsen]

Post by Paul Eggert

Post by Bruno Haible
It obviously does not handle the 'struct globnames' allocated with
the FLEXSIZEOF macro (lines 1719..1732).

Yes, and I installed the attached patch into Gnulib to try to fix this.
I hope it is enough to pacify clang's Undefined Sanitizer. If not, Tim,
please let us know, and thanks for reporting the problem.

Thanks for the patch !

I confirm that the sanitizer doesn't trigger any more with the latest gnulib.

Regards, Tim

[Bruno Haible]

Hi,

Post by Marc Nieper-Wißkirchen
In freea in malloca.c, a possibly uninitialized indicator word is used for
a comparison so that Valgrind reports: "Conditional jump or move depends on
uninitialised value(s)".
Valgrind is not smart enough to understand the logic in freea.
It would be nice if the warning could be silenced

It can be silenced: The Gnulib 'malloca' module comes with a file
'malloca.valgrind'. Use a valgrind command-line option such as

--suppressions=$(srcdir)/gnulib-lib/malloca.valgrind

You need to adjust the path to the file, to match the directory structure of
your package.

Bruno