develooper Front page | perl.perl5.porters | Postings from December 2017

Re: [perl #132234] use-of-uninitialized-value in Perl_upg_version(vutil.c:669)

Thread Previous | Thread Next
December 9, 2017 23:36
Re: [perl #132234] use-of-uninitialized-value in Perl_upg_version(vutil.c:669)
Message ID:
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);

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

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


Thread Previous | Thread Next Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at | Group listing | About