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

[Perl/perl5] 6a2333: Avoid leaking a scalar body after REGEXP toPVLV a...

Thread Next
From:
Nicholas Clark via perl5-changes
Date:
September 22, 2021 13:10
Subject:
[Perl/perl5] 6a2333: Avoid leaking a scalar body after REGEXP toPVLV a...
Message ID:
Perl/perl5/push/refs/heads/manicheck/000000-822080@github.com
  Branch: refs/heads/manicheck
  Home:   https://github.com/Perl/perl5
  Commit: 6a2333c434839d402a18986ca4fde7afcc0e181f
      https://github.com/Perl/perl5/commit/6a2333c434839d402a18986ca4fde7afcc0e181f
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-11 (Sat, 11 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: f6fc6bfa3b35d293be2eeab609c36b306a915a93
      https://github.com/Perl/perl5/commit/f6fc6bfa3b35d293be2eeab609c36b306a915a93
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-11 (Sat, 11 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: bb57326e197ec95362282a19be549d9d1fa5d390
      https://github.com/Perl/perl5/commit/bb57326e197ec95362282a19be549d9d1fa5d390
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-17 (Fri, 17 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: aa7c1f8cf7cbc95b74a15157510b55ed20e2e025
      https://github.com/Perl/perl5/commit/aa7c1f8cf7cbc95b74a15157510b55ed20e2e025
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-17 (Fri, 17 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: 960d9e02c925090cd94e6fff3c45209b55b2ff62
      https://github.com/Perl/perl5/commit/960d9e02c925090cd94e6fff3c45209b55b2ff62
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-17 (Fri, 17 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: 91c11aa1a6ea6d5ff04bb3d9d26562f46ed8bb5a
      https://github.com/Perl/perl5/commit/91c11aa1a6ea6d5ff04bb3d9d26562f46ed8bb5a
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-17 (Fri, 17 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: 6360dd5dfbef02658f56d9b205c52e55363160aa
      https://github.com/Perl/perl5/commit/6360dd5dfbef02658f56d9b205c52e55363160aa
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-17 (Fri, 17 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: 06068af26c895a212d9724f5f048c9cc4413a156
      https://github.com/Perl/perl5/commit/06068af26c895a212d9724f5f048c9cc4413a156
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-17 (Fri, 17 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: 7c79d186c2a98548b853fa3fc99988e543121501
      https://github.com/Perl/perl5/commit/7c79d186c2a98548b853fa3fc99988e543121501
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-17 (Fri, 17 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: 20a103fcd5512fbe1a0d01dcdbffb19c573b5933
      https://github.com/Perl/perl5/commit/20a103fcd5512fbe1a0d01dcdbffb19c573b5933
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-17 (Fri, 17 Sep 2021)

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

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


  Commit: 3b5affc82f27fb2be041f852f1029f2480209bfe
      https://github.com/Perl/perl5/commit/3b5affc82f27fb2be041f852f1029f2480209bfe
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-17 (Fri, 17 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: 55670c2b6b402b4fba072150c3b34c899ed58bc0
      https://github.com/Perl/perl5/commit/55670c2b6b402b4fba072150c3b34c899ed58bc0
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-20 (Mon, 20 Sep 2021)

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

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


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

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

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


  Commit: 84b095cd24a9b6623a1f7693b67cfb2b0e774798
      https://github.com/Perl/perl5/commit/84b095cd24a9b6623a1f7693b67cfb2b0e774798
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-20 (Mon, 20 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: ba1bfac74e6888d7917da19e992cb991e6458aed
      https://github.com/Perl/perl5/commit/ba1bfac74e6888d7917da19e992cb991e6458aed
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-20 (Mon, 20 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: e3a559528855ecacd1fd72a296a40e1651c70753
      https://github.com/Perl/perl5/commit/e3a559528855ecacd1fd72a296a40e1651c70753
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-20 (Mon, 20 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: 87e639c1912be6f88075e137af3ab28d00f0966f
      https://github.com/Perl/perl5/commit/87e639c1912be6f88075e137af3ab28d00f0966f
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-20 (Mon, 20 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: e238eb94c7efc1690955f2b0d5a1c3208c367df1
      https://github.com/Perl/perl5/commit/e238eb94c7efc1690955f2b0d5a1c3208c367df1
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-20 (Mon, 20 Sep 2021)

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

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


  Commit: 3999cdef064cad7ae38b0f3488dce424538b397b
      https://github.com/Perl/perl5/commit/3999cdef064cad7ae38b0f3488dce424538b397b
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-21 (Tue, 21 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.


  Commit: 166177f333b0bcca1c98a0346a1c55e55f800bb6
      https://github.com/Perl/perl5/commit/166177f333b0bcca1c98a0346a1c55e55f800bb6
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-22 (Wed, 22 Sep 2021)

  Changed paths:
    M Porting/manicheck

  Log Message:
  -----------
  In manicheck be explicit that the code is stripping './'

The regex had been written as /^../, which strictly was correct as the
string passed would *always* start with './', but this wasn't clear.

One had to stop and double check how File::Find::find() works, and what
argument it was called with. And it's not exactly clear when the '.' in
the regex text matches '.' and then '/', but the '/' in the regex is the
delimiter.

Given that the regex is being changed, move it later - previously the code
was performing a substitution on the value of $File::Find::name before it
knew that it needed it. (ie doing work for all directories.)


  Commit: 2ded4a4ae1d1d063789655aaa857656719f6c282
      https://github.com/Perl/perl5/commit/2ded4a4ae1d1d063789655aaa857656719f6c282
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-22 (Wed, 22 Sep 2021)

  Changed paths:
    M Porting/manicheck

  Log Message:
  -----------
  manicheck now optionally exits non-zero if it finds problems

This will enable it to be used in a CI test.

Refactor the code to avoid the temporary array @files by iterating over the
lines of MANIFEST as they are read in.


  Commit: 8220802a73219a70e91f98024e77d83919556275
      https://github.com/Perl/perl5/commit/8220802a73219a70e91f98024e77d83919556275
  Author: Nicholas Clark <nick@ccl4.org>
  Date:   2021-09-22 (Wed, 22 Sep 2021)

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

  Log Message:
  -----------
  Add CI tests for `make distclean` and Porting/manicheck


Compare: https://github.com/Perl/perl5/compare/6a2333c43483%5E...8220802a7321

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