Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use-of-uninitialized-value in Perl_upg_version (vutil.c:669) #16186

Open
p5pRT opened this issue Oct 6, 2017 · 6 comments
Open

use-of-uninitialized-value in Perl_upg_version (vutil.c:669) #16186

p5pRT opened this issue Oct 6, 2017 · 6 comments

Comments

@p5pRT
Copy link

p5pRT commented Oct 6, 2017

Migrated from rt.perl.org#132234 (status was 'open')

Searchable as RT132234$

@p5pRT
Copy link
Author

p5pRT commented Oct 6, 2017

From @geeknik

While building 1195d90 with -fsanitize=memory, the process fails during the
miniperl build.

afl-clang-fast [tpcg] 2.51b by <lszekeres@​google.com>
./miniperl -w -Ilib -Idist/Exporter/lib -MExporter -e '<?>' || sh -c 'echo

&2 Failed to build miniperl. Please run make minitest; exit 1'
==11441==WARNING​: MemorySanitizer​: use-of-uninitialized-value
  #0 0x560a10786ab7 in Perl_upg_version /root/perl/./vutil.c​:669​:2
  #1 0x560a107839a2 in Perl_new_version /root/perl/./vutil.c​:551​:12
  #2 0x560a10acb9f4 in S_require_version /root/perl/pp_ctl.c​:3663​:10
  #3 0x560a10acb9f4 in Perl_pp_require /root/perl/pp_ctl.c​:4291
  #4 0x560a10757a0d in Perl_runops_debug /root/perl/dump.c​:2486​:23
  #5 0x560a10432f4e in Perl_call_sv /root/perl/perl.c​:2921​:6
  #6 0x560a10419c56 in Perl_call_list /root/perl/perl.c​:5086​:6
  #7 0x560a103b2253 in S_process_special_blocks /root/perl/op.c​:9061​:6
  #8 0x560a1037a348 in Perl_newATTRSUB_x /root/perl/op.c​:8990​:21
  #9 0x560a1038434b in Perl_utilize /root/perl/op.c​:6341​:5
  #10 0x560a105c3ee4 in Perl_yyparse /root/perl/perly.y​:360​:6
  #11 0x560a104296f9 in S_parse_body /root/perl/perl.c​:2450​:9
  #12 0x560a1041dc2e in perl_parse /root/perl/perl.c​:1753​:2
  #13 0x560a10e00597 in main /root/perl/miniperlmain.c​:127​:18
  #14 0x7fa54f9ea82f in __libc_start_main
/build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c​:291
  #15 0x560a102b4e88 in _start (/root/perl/miniperl+0x2fe88)

SUMMARY​: MemorySanitizer​: use-of-uninitialized-value
/root/perl/./vutil.c​:669​:2 in Perl_upg_version
Exiting
Failed to build miniperl. Please run make minitest
makefile​:362​: recipe for target 'lib/buildcustomize.pl' failed
make​: *** [lib/buildcustomize.pl] Error 1

@p5pRT
Copy link
Author

p5pRT commented Oct 19, 2017

From @tonycoz

On Fri, 06 Oct 2017 13​:42​:55 -0700, brian.carpenter@​gmail.com wrote​:

While building 1195d90 with -fsanitize=memory, the process fails during the
miniperl build.

Could I have the Configure options and clang version you used to build this? (since you obviously can't run perl -V)

Thanks,
Tony

@p5pRT
Copy link
Author

p5pRT commented Oct 19, 2017

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Oct 19, 2017

From @geeknik

./Configure -des -Dusedevel -DDEBUGGING -Dcc=afl-clang-fast -Doptimize=-O2\
-g -Accflags='-fno-omit-frame-pointer -fsanitize=memory
-fsanitize-coverage=edge,trace-pc-guard,indirect-calls,trace-cmp,trace-div,trace-gep'
-Aldflags='-fno-omit-frame-pointer -fsanitize=memory
-fsanitize-coverage=edge,trace-pc-guard,indirect-calls,trace-cmp,trace-div,trace-gep'
&& AFL_PATH=/root/afl-2.51b make

Chromium clang version 6.0.0-trunk​: git clone
https://chromium.googlesource.com/chromium/src/tools/clang &&
clang/scripts/update.py

On Wed, Oct 18, 2017 at 7​:51 PM, Tony Cook via RT <perlbug-followup@​perl.org

wrote​:

On Fri, 06 Oct 2017 13​:42​:55 -0700, brian.carpenter@​gmail.com wrote​:

While building 1195d90 with -fsanitize=memory, the process fails during
the
miniperl build.

Could I have the Configure options and clang version you used to build
this? (since you obviously can't run perl -V)

Thanks,
Tony

@p5pRT
Copy link
Author

p5pRT commented Dec 9, 2017

From zefram@fysh.org

Brian Carpenter wrote​:

==11441==WARNING​: MemorySanitizer​: use-of-uninitialized-value
#0 0x560a10786ab7 in Perl_upg_version /root/perl/./vutil.c​:669​:2

I can't make sense of this, but I can see some other flaws in the
same code.

That bit of code is concerned with expanding an NV to a version object,
and more specifically with rendering the NV as a string. The process
it's trying to achieve is basically

  $_ = sprintf("%.9f", $nv);
  s/\.?0+\z//;

Line 669 is part of that trimming operation​:

  while (buf[len-1] == '0' && len > 0) len--;

One can immediately see a flaw, that the conditions are in the wrong
order. The "len > 0" is clearly concerned about running off the beginning
of the buffer, and ought to be tested before reading from buf[len-1].
If the memory immediately before buf counts as uninitialised, but is
mapped so accessing it doesn't segv, this would explain getting the
correct behaviour from this line while also getting this uninitialised
read.

But the logic of the situation is such that this trimming operation can't
ever reach the beginning of the string. The sprintf format means that
the trimmable trailing digits must always be preceded by the decimal
point, which itself must be preceded by at least one digit. Indeed,
the following line of code

  if ( buf[len-1] == '.' ) len--; /* eat the trailing decimal */

shows no worry about hitting the beginning of the string. And if I put in
assertions that check for len reaching zero, those assertions always pass,
not only in building but also in the entire test suite. I don't see how
the trimming operation could ever reach the beginning of the string.
So the "len > 0" on line 669 isn't so much misplaced as completely
extraneous. It should be removed, possibly replaced with assertions.

If those lines are OK, what about the preceding code that sets up buf
and len? There are two versions of this code, one using a stack-allocated
fixed-size buffer, and one using an SV. There's logic attempting to use
the fixed-size buffer if the result will fit, with fallback to the SV.
The use of "10e50" as a threshold for SV use suggests that the author
didn't quite understand "e" notation, but ultimately I believe the
threshold is adequate to avoid overrunning the fixed-size buffer.

The code that uses the fixed-size buffer goes further to avoid overrunning
it, by using my_snprintf() rather than straight sprintf(). This should
be unnecessary, given the previous logic. It also doesn't actually
avoid undefined behaviour, because len would still get the untruncated
string length. So using snprintf() rather than straight sprintf()
here isn't achieving much; either way it would be more useful to have an
assertion that truncation didn't happen. If I insert such an assertion,
it always passes, as we'd expect.

The code that uses an SV buffer is much safer, assuming we can rely
on the buffer management of the core SV code. However, it has an
entertaining bug, in that it constructs the SV initially with an undef
value and then uses sv_catpvf() to append the formatted string. So if
warnings are enabled in the relevant scope, the version upgrade produces
a bogus "uninitialized value" warning. (Try "require(2e51)".) If I
change the buffer choice logic to always go the SV route, hundreds of
such warnings are duly produced. Correcting sv_catpvf() to sv_setpvf()
resolves those warnings, and with that bugfix the always-SV version of
the function passes all tests. I believe that code is correct apart
from the undef issue.

So plenty of flaws there, but I don't see the one claimed by the bug
report.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Dec 10, 2017

From @cpansprout

On Sat, 09 Dec 2017 15​:36​:55 -0800, zefram@​fysh.org wrote​:

So plenty of flaws there, but I don't see the one claimed by the bug
report.

Whatever changes you might make, please make sure they get submitted to version.pm’s bug queue, and that they are compatible with older perl versions. (Many committers have thus far ignored the comment at the top of the file.)

--

Father Chrysostomos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants