develooper Front page | perl.perl5.changes | Postings from September 2021

[Perl/perl5] 07024c: Avoid leaking a scalar body after REGEXP toPVLV a...

Thread Next
From:
Nicholas Clark via perl5-changes
Date:
September 23, 2021 15:54
Subject:
[Perl/perl5] 07024c: Avoid leaking a scalar body after REGEXP toPVLV a...
Message ID:
Perl/perl5/push/refs/heads/smoke-me/nicholas/ASAN-PURIFY-github/000000-be54e7@github.com
  Branch: refs/heads/smoke-me/nicholas/ASAN-PURIFY-github
  Home:   https://github.com/Perl/perl5
  Commit: 07024caa60f04ae203801d222536bfdad4caf7e8
      https://github.com/Perl/perl5/commit/07024caa60f04ae203801d222536bfdad4caf7e8
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M sv.c

  Log Message:
  -----------
  Avoid leaking a scalar body after REGEXP to PVLV assignment

PVLV scalars are used to implement various obscure features of perl, such as
elements in tied hashes and hash lookups used as function parameters. To
make these work, PVLVs must be capable of holding all scalar types. First
class REGEXPs are larger than PVLVs, hence there is special case code to
attach REGEXP bodies to PVLVs, for code that both dereferences a regex object
and then assigns that value to a PVLV scalar. This was first implemented in
Oct 2012 by commit 8d919b0a35f2b57a:
    Allow regexp-to-pvlv assignment

and improved in July 2017 with commit df6b4bd56551f2d3:

It turns out that the implementation would leak a scalar body, for the
"normal" use case (of this obscure feature). This wasn't noticed for two
reasons

1) bodies are allocated from arenas, and arenas are correctly freed during
   full destruction.
2) the body is not leaked if the the PVLV scalar is then modified

The regression tests added by commit 8d919b0a35f2b57a were also testing
the corner case of concatenating to one of these values - ie that
sv_force_normal_flags() handled unwinding the special cases correctly.
To avoid code duplication, the tests added by that commit caused all PVLV
scalars to be passed to sv_force_normal_flags(), so didn't actually test
regular destruction of such scalars.

The tests added in Aug 2017 by commit 622c613e12cef84c:
    PVLV-as-REGEXP: avoid PVX double free

    With v5.27.2-30-gdf6b4bd, I changed the way that PVLVs store a regexp
    value (by making the xpv_len field point to a regexp struct). There was a
    bug in this, which caused the PVX buffer to be double freed.

    Several REGEXP SVs can share a PVX buffer. Only one of them will have a
    non-zero xpv_len field, and that SV gets to free the buffer.

    After the commit above, the non-zero xpv_len was triggering an extra free.

    This was showing up in smokes as failures in re/recompile.t when invoked
    with PERL_DESTRUCT_LEVEL=2 (which t/TEST does).

were actually the first to expose this bug, even though it had been present
since 2012.

The bug is only visible with a full leak test (eg valgrind --leak-check=full
or an ASAN build) with -DPURIFY (to disable arenas), hence why it hasn't
been noticed.


  Commit: d978f0698f3cdb11f60c849e5ec37c62f82c731a
      https://github.com/Perl/perl5/commit/d978f0698f3cdb11f60c849e5ec37c62f82c731a
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M embed.fnc
    M embed.h
    M hv.c
    M proto.h

  Log Message:
  -----------
  Don't leak in hv_common when croaking about PL_strtab

hv_common can perform read-only actions on PL_strtab, but not write actions.
The code that detects this and croaks had been just after the allocation of
a new HE *, and hence was leaking it. Re-order the code to avoid the leak.

The leak usually wasn't noticeable as HEs are allocated from arenas, and
arenas are correctly freed during full destruction. However, building with
-DPURIFY replaces arenas with individual allocations, making this leak
visible. It's unlikely to have been hit by any production code, but it was
causing leaks during the regression tests.

Also change embed.fnc so that S_new_HE's prototype is not declared under
-DPURIFY, as the static function itself is not defined in this case. This
fixes a compiler warning.


  Commit: a3b62bcfb703a5b6d2dea7b629ab4cad99e67772
      https://github.com/Perl/perl5/commit/a3b62bcfb703a5b6d2dea7b629ab4cad99e67772
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M inline.h
    M regcomp.c
    M sv.c

  Log Message:
  -----------
  Add comments describing how PVLVs store REGEXPs by reference


  Commit: 43a905e9eb819b330f633b64651b3bef4a54d740
      https://github.com/Perl/perl5/commit/43a905e9eb819b330f633b64651b3bef4a54d740
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M .github/workflows/testsuite.yml

  Log Message:
  -----------
  Add "missing" descriptions to testsuite.yml

If it's worth doing, it's worth overdoing.


  Commit: b24eee06bfdf0a8e1eef3c5b0eed025336807970
      https://github.com/Perl/perl5/commit/b24eee06bfdf0a8e1eef3c5b0eed025336807970
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M .github/workflows/testsuite.yml

  Log Message:
  -----------
  Add a CI job to confirm that minitest keeps passing

Currently minitest passes - let's keep it that way.

minitest might feel like makework, until one makes a seemingly innocent
change that breaks the build in "inconceivable" ways and it's no longer
even possible to get to the point of being able to run `make test`.


  Commit: 3679a3005777a8d77074e2d9a19a03b3744e1c66
      https://github.com/Perl/perl5/commit/3679a3005777a8d77074e2d9a19a03b3744e1c66
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M .github/workflows/testsuite.yml

  Log Message:
  -----------
  A prototype CI job to build with Address Sanitizer

Whilst this *will* detect various dangerous mistakes involving bad memory
accesses, it's far more likely to spot if changes create memory leaks.


  Commit: f67c22d1f597829601ebf2a3f22f9532f933b6b1
      https://github.com/Perl/perl5/commit/f67c22d1f597829601ebf2a3f22f9532f933b6b1
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M .github/workflows/testsuite.yml

  Log Message:
  -----------
  All workflow jobs use the same env: so move it to the top level

Also nothing references $ENV{WORKSPACE} so no need to set this. Documentation
for actions/checkout implies that it is reading from $GITHUB_WORKSPACE.


  Commit: bf1ac9e3308918246f2ecc6e610c22313c88c991
      https://github.com/Perl/perl5/commit/bf1ac9e3308918246f2ecc6e610c22313c88c991
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M .github/workflows/testsuite.yml

  Log Message:
  -----------
  Workflows should `make test_harness` to get parallel testing

`make test` uses t/TEST which always runs tests sequentially.
TEST_JOBS only makes sense with t/harness, which is run by the
`test_harness` target.


  Commit: 79fbd4eeb88185cac5c12271c4c9a92f13649409
      https://github.com/Perl/perl5/commit/79fbd4eeb88185cac5c12271c4c9a92f13649409
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M .github/workflows/testsuite.yml

  Log Message:
  -----------
  Test ASAN builds with and without -DPURIFY

Also we can fail-fast on ASAN to save some resources. Likely if one ASAN
build fails, the others won't tell us about different problems.


  Commit: 4e56f98ea6c259ef23621664d6ab83809c8e1c0d
      https://github.com/Perl/perl5/commit/4e56f98ea6c259ef23621664d6ab83809c8e1c0d
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M .github/workflows/testsuite.yml

  Log Message:
  -----------
  In the Linux workflows use MALLOC_PERTURB_ and MALLOC_CHECK_

MALLOC_CHECK_=3 causes glibc to abort if it detects malloc errors.
Specifically, 3 will be "Print detailed error message, stack trace, and
memory mappings, and abort the program."

MALLOC_PERTURB_=254 will cause newly allocated memory to be set to non-zero
values, and freed memory to be set to the given non-zero value.

See `man mallopt`

Intentionally only setting these for the build and test steps, but not the
Configure step, to see if we can later expose bugs caused by bad probes.
If we set it for Configure, the abort this triggers might simply cause a
probe to be silently failed, and we could miss problems.


  Commit: 7ce4d5f23ec1cfaca6d5813b628aa0ecb3c3e05f
      https://github.com/Perl/perl5/commit/7ce4d5f23ec1cfaca6d5813b628aa0ecb3c3e05f
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M .github/workflows/testsuite.yml

  Log Message:
  -----------
  CI tests for PERL_UNICODE="" with a UTF-8 locale


  Commit: 45b3833c1fceddd7d5ed20ffe36258fa931de51e
      https://github.com/Perl/perl5/commit/45b3833c1fceddd7d5ed20ffe36258fa931de51e
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M .github/workflows/testsuite.yml
    M .travis.yml

  Log Message:
  -----------
  PERL_UNICODE= and LANG= can't be set as Configure arguments

The intent is to test with non-C locales and non-default Unicode handling,
but to do this the values need to be set as environment variables for the
test (and build - occasionally bugs are not exposed by the testsuite).

Configure happily accepts anything on the command line, and records all its
command line arguments in config.sh (and hence also within %Config::Config).
But nothing acts on them.

It's easier to set these in a separate workflow job (and test only a couple)
rather than creating a second environment matrix in this job, and hence
test $m * $n combinations.

Unfortunately the GH test machines only have the locales C, C.UTF-8, POSIX
and en_US.utf8, so we can't easily test anything more "interesting" than
en_US.utf8.


  Commit: 9cf90a6232e9f412517dfb44f145e8bbb4539551
      https://github.com/Perl/perl5/commit/9cf90a6232e9f412517dfb44f145e8bbb4539551
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M .github/workflows/testsuite.yml

  Log Message:
  -----------
  The CI workflow needs some test builds without -DDEBUGGING


  Commit: d659bdcfb63f693b1a518ee362b5331576d053b0
      https://github.com/Perl/perl5/commit/d659bdcfb63f693b1a518ee362b5331576d053b0
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M .github/workflows/testsuite.yml

  Log Message:
  -----------
  Test `make install` in the GH workflow


  Commit: a83e3f5d82002d8d0772bf56b494fd3fab6423a3
      https://github.com/Perl/perl5/commit/a83e3f5d82002d8d0772bf56b494fd3fab6423a3
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M .github/workflows/testsuite.yml

  Log Message:
  -----------
  Run `./perl t/harness` directly in the CI workflow

`make -j2 test_harness` causes make to re-run the build in all the extension
directories, which can take 20 seconds even on fast server hardware. Hence
change the build target to `test_prep`, the Makefile pre-requisite for
testing, and then run t/harness directly with the same environment as the
Makefile and ./runtests would have set up. As we already set
PERL_SKIP_TTY_TEST=1 in the global environment we don't have anything else
to set up.

Keep the 'linux-i386' and 'install' jobs on the regular `test` and
`test_harness` targets to ensure we still cover them.


  Commit: 36cdf6603582b118ffb6a8ef20a07f814c0a0864
      https://github.com/Perl/perl5/commit/36cdf6603582b118ffb6a8ef20a07f814c0a0864
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M .github/workflows/testsuite.yml

  Log Message:
  -----------
  Change all LD_LIBRARY_PATH setup from '.' to `pwd`

The previous commit added various new lines that set LD_LIBRARY_PATH=`pwd`,
whereas the existing lines that set LD_LIBRARY_PATH set it to '.'.

LD_LIBRARY_PATH needs to be set to the build directory to run an uninstalled
perl binary *if* that binary is built with a shared perl library. It's not
needed for the default configuration, but to simplify the test definitions we
were doing it always.

In turn, LD_LIBRARY_PATH doesn't need to be set to an *absolute* path if all
that is invoked is the perl binary - it doesn't change directory, and it
does shell out to itself, hence setting LD_LIBRARY_PATH=. is just fine here.

Running the regression tests *does* involve changing directory before
invoking perl, so LD_LIBRARY_PATH needs to be set to an absolute path there.
Running `./perl -Ilib -V` doesn't change directory (or shell out) so '.'
was good enough.

There's no real harm in also invoking `pwd` to get an absolute path here, so
do this instead of just coding '.', as the consistency makes the workflow
easier to read - you don't get distracted wondering "Why are these
different, and is that a bug?"


  Commit: 5357f462fdfc29f671510a60e138de9f31cf5e0e
      https://github.com/Perl/perl5/commit/5357f462fdfc29f671510a60e138de9f31cf5e0e
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M .github/workflows/testsuite.yml

  Log Message:
  -----------
  Run t/TEST directly on cygwin in the CI workflow

`make -j2 test` causes make to re-run the build in all the extension
directories, which takes about 40 seconds on the GH servers - ie this alone
is roughly 1% of the total test time.

Hence change the build target to `test_prep`, the Makefile pre-requisite for
testing, and then run t/TEST.pl directly with the same environment as the
Makefile and ./runtests would have set up. As we already set
PERL_SKIP_TTY_TEST=1 in the global environment we don't have anything else
to set up.


  Commit: 1c6f608fb4cb8723da36597ce023483c5c271ae7
      https://github.com/Perl/perl5/commit/1c6f608fb4cb8723da36597ce023483c5c271ae7
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M .github/workflows/testsuite.yml

  Log Message:
  -----------
  Test clang's ASAN as well as gcc's ASAN

On the GH virtual machine, gcc's ASAN appears to be slightly buggy, as the
`-Accflags=-DPURIFY` job is causing cpan/File-Path/t/Path.t to reliably
SEGV after passing all 167 subtests:
    Tracer caught signal 11: addr=0x0 pc=0x7ff95dfaa8b0 sp=0x7ff95a2a6ce0
    LeakSanitizer has encountered a fatal error.

I can't replicate this on machines I have access to, which makes it rather
hard to debug. Hence side-step the issue by running this test with clang
instead, so that we also test with clang's ASAN implementation.

We still test with gcc's ASAN in one of the PERL_UNICODE jobs.


  Commit: a29a7161711a02b27ff184e23ccfdea39df48a7a
      https://github.com/Perl/perl5/commit/a29a7161711a02b27ff184e23ccfdea39df48a7a
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M .github/workflows/testsuite.yml

  Log Message:
  -----------
  Test a macOS threaded shared-library build too


  Commit: be54e79b6a3ad5913e8bae20aa0748c351b916b4
      https://github.com/Perl/perl5/commit/be54e79b6a3ad5913e8bae20aa0748c351b916b4
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-23 (Thu, 23 Sep 2021)

  Changed paths:
    M .github/workflows/testsuite.yml

  Log Message:
  -----------
  Test ASAN with -DDEBUGGING (and so also with assertions enabled)

Make it clear in the earlier comment about -DDEBUGGING that it was
referencing the "linux" job, not the entire file.


Compare: https://github.com/Perl/perl5/compare/07024caa60f0%5E...be54e79b6a3a

Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About