Storable and Leaky hooks which die

Storage has some tests which leak, and thus cause smoke failures on blead
when run under Address Sanitizer.

An example of code which leaks is:

    use Storable qw(store);
    sub FreezeHookDies::STORABLE_freeze { die ${$_[0]} }
    my $x = bless [], "FreezeHookDies";
    eval { store($x, "store99"); 1 };

I had a quick look at the source (which I'm not very familiar with)
and decided it was non-trivial and non-obvious what the best way to fix
it is. It's in the general category of mallocing temp buffers pointed
to be local vars, which don't get freed if the code dies (and is caught by
an eval).

Since it looks too messy to fix for 5.30.0 I have, for now, with
v5.29.9-133-g2cf7500760, skipped the leaky tests when run under ASan,
but the skip will auto-re-enable in 5.31.x

Dopes anyone more familiar with the internals of Storable want to have a
go at fixing it, or at least suggest the best approach?

-- 
"I used to be with it, but then they changed what ‘it’ was, and now what
I’m with isn’t it. And what’s ‘it’ seems weird and scary to me."
  -- Grandpa Simpson
(It will happen to you too.)
0
davem
4/15/2019 10:48:13 AM
perl.perl5.porters 47579 articles. 1 followers. Follow

7 Replies
8 Views

Similar Articles

[PageSpeed] 3

On Mon, Apr 15, 2019 at 11:48:13AM +0100, Dave Mitchell wrote:
> Storage has some tests which leak, and thus cause smoke failures on blead
> when run under Address Sanitizer.
> 
> An example of code which leaks is:
> 
>     use Storable qw(store);
>     sub FreezeHookDies::STORABLE_freeze { die ${$_[0]} }
>     my $x = bless [], "FreezeHookDies";
>     eval { store($x, "store99"); 1 };
> 
> I had a quick look at the source (which I'm not very familiar with)
> and decided it was non-trivial and non-obvious what the best way to fix
> it is. It's in the general category of mallocing temp buffers pointed
> to be local vars, which don't get freed if the code dies (and is caught by
> an eval).
> 
> Since it looks too messy to fix for 5.30.0 I have, for now, with
> v5.29.9-133-g2cf7500760, skipped the leaky tests when run under ASan,
> but the skip will auto-re-enable in 5.31.x
> 
> Dopes anyone more familiar with the internals of Storable want to have a
> go at fixing it, or at least suggest the best approach?

I'll take a look.

Tony
0
tony
4/16/2019 5:35:43 AM
On Mon, Apr 15, 2019 at 11:48:13AM +0100, Dave Mitchell wrote:
> Storage has some tests which leak, and thus cause smoke failures on blead
> when run under Address Sanitizer.
> 
> An example of code which leaks is:
> 
>     use Storable qw(store);
>     sub FreezeHookDies::STORABLE_freeze { die ${$_[0]} }
>     my $x = bless [], "FreezeHookDies";
>     eval { store($x, "store99"); 1 };
> 
> I had a quick look at the source (which I'm not very familiar with)
> and decided it was non-trivial and non-obvious what the best way to fix
> it is. It's in the general category of mallocing temp buffers pointed
> to be local vars, which don't get freed if the code dies (and is caught by
> an eval).
> 
> Since it looks too messy to fix for 5.30.0 I have, for now, with
> v5.29.9-133-g2cf7500760, skipped the leaky tests when run under ASan,
> but the skip will auto-re-enable in 5.31.x
> 
> Dopes anyone more familiar with the internals of Storable want to have a
> go at fixing it, or at least suggest the best approach?

For some reason I'm not seeing any leaks from an ASAN build.

 ./Configure -des -Dusedevel -Accflags=-fsanitize=address -Aldflags=-fsanitize=address -DDEBUGGING -Doptimize=-O0\ -g -Dcc=/opt/gcc-8.2.0/bin/gcc

valgrind picked up a leak of the ptr table from your example which is
fixed in 1d7b2a7e3a7a0e05c254d065923488b768eb3ce0.

Tony
0
tony
4/17/2019 1:18:26 AM
On Wed, 17 Apr 2019 11:18:26 +1000
Tony Cook <tony@develop-help.com> wrote:

> For some reason I'm not seeing any leaks from an ASAN build.
> 
>  ./Configure -des -Dusedevel -Accflags=-fsanitize=address -Aldflags=-fsanitize=address -DDEBUGGING -Doptimize=-O0\ -g -Dcc=/opt/gcc-8.2.0/bin/gcc
> 
> valgrind picked up a leak of the ptr table from your example which is
> fixed in 1d7b2a7e3a7a0e05c254d065923488b768eb3ce0.
> 
> Tony

To enable leak detection in ASAN you need -fsanitize=leak flag.
-fsanitize=address is just for memory errors.
0
me
4/17/2019 2:00:32 AM
On Wed, Apr 17, 2019 at 04:00:32AM +0200, Tomasz Konojacki wrote:
> On Wed, 17 Apr 2019 11:18:26 +1000
> Tony Cook <tony@develop-help.com> wrote:
> 
> > For some reason I'm not seeing any leaks from an ASAN build.
> > 
> >  ./Configure -des -Dusedevel -Accflags=-fsanitize=address -Aldflags=-fsanitize=address -DDEBUGGING -Doptimize=-O0\ -g -Dcc=/opt/gcc-8.2.0/bin/gcc
> > 
> > valgrind picked up a leak of the ptr table from your example which is
> > fixed in 1d7b2a7e3a7a0e05c254d065923488b768eb3ce0.
> > 
> > Tony
> 
> To enable leak detection in ASAN you need -fsanitize=leak flag.
> -fsanitize=address is just for memory errors.

Yeah, I tried that too (ie. both address and leak).

https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer

implies that -fsanitize=leak isn't needed if you have
-fsanitize=address.

Tony
0
tony
4/17/2019 2:12:49 AM
On 4/16/19 8:12 PM, Tony Cook wrote:
> On Wed, Apr 17, 2019 at 04:00:32AM +0200, Tomasz Konojacki wrote:
>> On Wed, 17 Apr 2019 11:18:26 +1000
>> Tony Cook <tony@develop-help.com> wrote:
>>
>>> For some reason I'm not seeing any leaks from an ASAN build.
>>>
>>>   ./Configure -des -Dusedevel -Accflags=-fsanitize=address -Aldflags=-fsanitize=address -DDEBUGGING -Doptimize=-O0\ -g -Dcc=/opt/gcc-8.2.0/bin/gcc
>>>
>>> valgrind picked up a leak of the ptr table from your example which is
>>> fixed in 1d7b2a7e3a7a0e05c254d065923488b768eb3ce0.
>>>
>>> Tony
>>
>> To enable leak detection in ASAN you need -fsanitize=leak flag.
>> -fsanitize=address is just for memory errors.
> 
> Yeah, I tried that too (ie. both address and leak).
> 
> https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer
> 
> implies that -fsanitize=leak isn't needed if you have
> -fsanitize=address.
> 
> Tony
> 

But it also says you need to set
  export ASAN_OPTIONS=detect_leaks=1
0
public
4/17/2019 3:02:11 AM
On Tue, Apr 16, 2019 at 09:02:11PM -0600, Karl Williamson wrote:
> On 4/16/19 8:12 PM, Tony Cook wrote:
> > On Wed, Apr 17, 2019 at 04:00:32AM +0200, Tomasz Konojacki wrote:
> > > On Wed, 17 Apr 2019 11:18:26 +1000
> > > Tony Cook <tony@develop-help.com> wrote:
> > > 
> > > > For some reason I'm not seeing any leaks from an ASAN build.
> > > > 
> > > >   ./Configure -des -Dusedevel -Accflags=-fsanitize=address -Aldflags=-fsanitize=address -DDEBUGGING -Doptimize=-O0\ -g -Dcc=/opt/gcc-8.2.0/bin/gcc
> > > > 
> > > > valgrind picked up a leak of the ptr table from your example which is
> > > > fixed in 1d7b2a7e3a7a0e05c254d065923488b768eb3ce0.
> > > > 
> > > > Tony
> > > 
> > > To enable leak detection in ASAN you need -fsanitize=leak flag.
> > > -fsanitize=address is just for memory errors.
> > 
> > Yeah, I tried that too (ie. both address and leak).
> > 
> > https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer
> > 
> > implies that -fsanitize=leak isn't needed if you have
> > -fsanitize=address.
> > 
> > Tony
> > 
> 
> But it also says you need to set
>  export ASAN_OPTIONS=detect_leaks=1

I use the clang that comes with Fedora 29:

    $ clang --version
    clang version 7.0.1 (Fedora 7.0.1-6.fc29)

With that version, leak detection is enabled by default with
-fsanitize=address, and until very recently I was manually disabling it
with 'export ASAN_OPTIONS=detect_leaks=0'

My full invocation is

$ export PERL_DESTRUCT_LEVEL=2
$ ( sh Configure -des -Dusedevel -Dprefix=/home/davem/perl5/git/bleed.out -Uinstallusrbinperl -Duseithreads -Doptimize='-g'  -Accflags='-DDEBUGGING -ggdb -fsanitize=address' -Aldflags='-fsanitize=address' -Dcc=clang && TEST_JOBS=8  HARNESS_OPTIONS=j8  make  -j 8 test_harness) > stdout.log 2> stderr.log

However, I note that Tony's Storage patch seems to have fixed the issue
ASan was complaining about. So I'll revert my "skip leaky Storable tests"
commit.

I've been working to make blead clean under ASan leak detection - so far
just under a threaded debugging build. I'm just left some with DB_File
failures.  It's possible that other build options may still leak. It's
also possible that valgrind will spot issues (or false positives) that ASan
doesn't and vice/versa. Karl has recently pointed out to me that
t/comp/colon.t fails under valgrind, although I haven't looked at that in
detail yet.


-- 
"I do not resent criticism, even when, for the sake of emphasis,
it parts for the time with reality".
    -- Winston Churchill, House of Commons, 22nd Jan 1941.
0
davem
4/17/2019 8:41:38 AM
On 2019-04-17, Tony Cook <tony@develop-help.com> wrote:
> On Mon, Apr 15, 2019 at 11:48:13AM +0100, Dave Mitchell wrote:
>> Storage has some tests which leak, and thus cause smoke failures on blead
>> when run under Address Sanitizer.
>> 
>> An example of code which leaks is:
>> 
>>     use Storable qw(store);
>>     sub FreezeHookDies::STORABLE_freeze { die ${$_[0]} }
>>     my $x = bless [], "FreezeHookDies";
>>     eval { store($x, "store99"); 1 };
>> 
>> I had a quick look at the source (which I'm not very familiar with)
>> and decided it was non-trivial and non-obvious what the best way to fix
>> it is. It's in the general category of mallocing temp buffers pointed
>> to be local vars, which don't get freed if the code dies (and is caught by
>> an eval).
>> 
>> Since it looks too messy to fix for 5.30.0 I have, for now, with
>> v5.29.9-133-g2cf7500760, skipped the leaky tests when run under ASan,
>> but the skip will auto-re-enable in 5.31.x
>> 
>> Dopes anyone more familiar with the internals of Storable want to have a
>> go at fixing it, or at least suggest the best approach?
>
> For some reason I'm not seeing any leaks from an ASAN build.
>
>  ./Configure -des -Dusedevel -Accflags=-fsanitize=address -Aldflags=-fsanitize=address -DDEBUGGING -Doptimize=-O0\ -g -Dcc=/opt/gcc-8.2.0/bin/gcc
>
> valgrind picked up a leak of the ptr table from your example which is
> fixed in 1d7b2a7e3a7a0e05c254d065923488b768eb3ce0.
>
While the patch seems correct, it does not fix the issue for me. When
I run the code under gdb, the storable_free() is never called. valgrind
reports these issues after your patch:

==12089== 9 bytes in 1 blocks are possibly lost in loss record 15 of 835
==12089==    at 0x483880B: malloc (vg_replace_malloc.c:309)
==12089==    by 0x4AF5E1: Perl_safesysmalloc (util.c:153)
==12089==    by 0x4AFAB6: Perl_savepvn (util.c:1084)
==12089==    by 0x4E8210: Perl_sv_magicext (sv.c:5733)
==12089==    by 0x4ECCB3: Perl_sv_magic (sv.c:5834)
==12089==    by 0x44E0E1: Perl_Gv_AMupdate (gv.c:2892)
==12089==    by 0x44E438: Perl_amagic_call (gv.c:3138)
==12089==    by 0x44FEBD: Perl_amagic_deref_call (gv.c:3072)
==12089==    by 0x4F8F3B: Perl_pp_rv2sv (pp.c:263)
==12089==    by 0x4CE285: Perl_runops_standard (run.c:41)
==12089==    by 0x43E954: Perl_call_sv (perl.c:3026)
==12089==    by 0x52EECC4: array_call (Storable.xs:2224)
==12089==    by 0x52EECC4: store_hook (Storable.xs:3751)
==12089==    by 0x52EECC4: store_blessed (Storable.xs:4162)

==12089== 16 bytes in 1 blocks are possibly lost in loss record 70 of 835
==12089==    at 0x483880B: malloc (vg_replace_malloc.c:309)
==12089==    by 0x4AF5E1: Perl_safesysmalloc (util.c:153)
==12089==    by 0x4D8381: Perl_pp_entersub (pp_hot.c:5142)
==12089==    by 0x43EA1B: Perl_call_sv (perl.c:3026)
==12089==    by 0x52EECC4: array_call (Storable.xs:2224)
==12089==    by 0x52EECC4: store_hook (Storable.xs:3751)
==12089==    by 0x52EECC4: store_blessed (Storable.xs:4162)
==12089==    by 0x52F1556: store (Storable.xs:4492)
==12089==    by 0x52F2B4B: do_store.isra.8 (Storable.xs:4685)
==12089==    by 0x52F2FCF: XS_Storable_pstore (Storable.xs:7829)

==12089== 48 bytes in 1 blocks are possibly lost in loss record 283 of 835
==12089==    at 0x483880B: malloc (vg_replace_malloc.c:309)
==12089==    by 0x4AF5E1: Perl_safesysmalloc (util.c:153)
==12089==    by 0x4DE83A: Perl_ptr_table_new (sv.c:13810)
==12089==    by 0x52F2A08: init_store_context (Storable.xs:1625)
==12089==    by 0x52F2A08: do_store.isra.8 (Storable.xs:4674)
==12089==    by 0x52F2FCF: XS_Storable_pstore (Storable.xs:7829)

==12089== 48 bytes in 1 blocks are possibly lost in loss record 284 of 835
==12089==    at 0x483AB1A: calloc (vg_replace_malloc.c:762)
==12089==    by 0x4B038C: Perl_safesyscalloc (util.c:439)
==12089==    by 0x4E816A: Perl_sv_magicext (sv.c:5685)
==12089==    by 0x4ECCB3: Perl_sv_magic (sv.c:5834)
==12089==    by 0x44E0E1: Perl_Gv_AMupdate (gv.c:2892)
==12089==    by 0x44E438: Perl_amagic_call (gv.c:3138)
==12089==    by 0x44FEBD: Perl_amagic_deref_call (gv.c:3072)
==12089==    by 0x4F8F3B: Perl_pp_rv2sv (pp.c:263)
==12089==    by 0x4CE285: Perl_runops_standard (run.c:41)
==12089==    by 0x43E954: Perl_call_sv (perl.c:3026)
==12089==    by 0x52EECC4: array_call (Storable.xs:2224)
==12089==    by 0x52EECC4: store_hook (Storable.xs:3751)
==12089==    by 0x52EECC4: store_blessed (Storable.xs:4162)
==12089==    by 0x52F1556: store (Storable.xs:4492)

==12089== 56 bytes in 1 blocks are possibly lost in loss record 321 of 835
==12089==    at 0x483AD19: realloc (vg_replace_malloc.c:836)
==12089==    by 0x4AFBA9: Perl_safesysrealloc (util.c:271)
==12089==    by 0x4E693A: Perl_sv_grow (sv.c:1596)
==12089==    by 0x4DFE90: S_sv_catpvn_simple (sv.c:10996)
==12089==    by 0x4DFE90: Perl_sv_vcatpvfn_flags (sv.c:12021)
==12089==    by 0x4E3BD1: Perl_sv_catpvf (sv.c:10896)
==12089==    by 0x4B0875: Perl_mess_sv (util.c:1435)
==12089==    by 0x4B00F2: Perl_vcroak (util.c:1695)
==12089==    by 0x4B0293: Perl_die (util.c:1630)
==12089==    by 0x4F9037: Perl_pp_rv2sv (pp.c:268)
==12089==    by 0x4CE285: Perl_runops_standard (run.c:41)
==12089==    by 0x43E954: Perl_call_sv (perl.c:3026)
==12089==    by 0x52EECC4: array_call (Storable.xs:2224)
==12089==    by 0x52EECC4: store_hook (Storable.xs:3751)
==12089==    by 0x52EECC4: store_blessed (Storable.xs:4162)

==12089== 64 bytes in 1 blocks are possibly lost in loss record 335 of 835
==12089==    at 0x483AB1A: calloc (vg_replace_malloc.c:762)
==12089==    by 0x4B038C: Perl_safesyscalloc (util.c:439)
==12089==    by 0x4C9368: Perl_hv_common (hv.c:590)
==12089==    by 0x4CA743: Perl_hv_common_key_len (hv.c:339)
==12089==    by 0x52EBDAB: pkg_fetchmeth (Storable.xs:2053)
==12089==    by 0x52EBDAB: pkg_can (Storable.xs:2133)
==12089==    by 0x52EE976: store_blessed (Storable.xs:4160)
==12089==    by 0x52F1556: store (Storable.xs:4492)
==12089==    by 0x52F2B4B: do_store.isra.8 (Storable.xs:4685)
==12089==    by 0x52F2FCF: XS_Storable_pstore (Storable.xs:7829)

==12089== 4,096 bytes in 1 blocks are possibly lost in loss record 762 of 835
==12089==    at 0x483AB1A: calloc (vg_replace_malloc.c:762)
==12089==    by 0x4B038C: Perl_safesyscalloc (util.c:439)
==12089==    by 0x4DE874: Perl_ptr_table_new (sv.c:13816)
==12089==    by 0x52F2A08: init_store_context (Storable.xs:1625)
==12089==    by 0x52F2A08: do_store.isra.8 (Storable.xs:4674)
==12089==    by 0x52F2FCF: XS_Storable_pstore (Storable.xs:7829)

==12089== 8,192 bytes in 1 blocks are possibly lost in loss record 801 of 835
==12089==    at 0x483880B: malloc (vg_replace_malloc.c:309)
==12089==    by 0x4AF5E1: Perl_safesysmalloc (util.c:153)
==12089==    by 0x4DEAA4: Perl_ptr_table_store (sv.c:13872)
==12089==    by 0x52F14D5: store (Storable.xs:4472)
==12089==    by 0x52F2B4B: do_store.isra.8 (Storable.xs:4685)
==12089==    by 0x52F2FCF: XS_Storable_pstore (Storable.xs:7829)

(There are other leaks pertaining boot_Storable() that I ignored).

Let's look at the last report: do_store() calls store() at 4685 and store()
makes a long jump because of the die exception. Thus do_store() never
advances to clean_store_context() at 4717 that would free the
cxt->pseen. The cxt is probably supposed to be deallocated when
unwinding a stack while handling the exception. However, I have no
idea how it is this implemented in Perl.

-- Petr
0
ppisar
4/17/2019 11:33:34 AM
Reply: