In perl.git, the branch blead has been updated <http://perl5.git.perl.org/perl.git/commitdiff/da1ec2b1b9abdfd956d9c539abf39d908d046304?hp=90c3aa01208e3c5b9ab464a058bbd2f6ebda4ff4>
- Log ----------------------------------------------------------------- commit da1ec2b1b9abdfd956d9c539abf39d908d046304 Author: Tony Cook <[email protected]> Date: Mon Feb 6 11:38:10 2017 +1100 prevent leak of class name from retrieve_hook() on an exception If supplied with a large class name, retrieve_hook() allocates buffer for the class name and Safefree()s it on exit path. Unfortunately this memory leaks if load_module() (or a couple of other code paths) throw an exception. So use SAVEFREEPV() to release the memory instead. ==20183== 193 bytes in 1 blocks are definitely lost in loss record 4 of 6 ==20183== at 0x4C28C20: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==20183== by 0x55F85D: Perl_safesysmalloc (util.c:153) ==20183== by 0x6ACA046: retrieve_hook (Storable.xs:4265) ==20183== by 0x6AD6D19: retrieve (Storable.xs:6217) ==20183== by 0x6AD8144: do_retrieve (Storable.xs:6401) ==20183== by 0x6AD85B7: pretrieve (Storable.xs:6506) ==20183== by 0x6AD8E14: XS_Storable_pretrieve (Storable.xs:6718) ==20183== by 0x5C176D: Perl_pp_entersub (pp_hot.c:4227) ==20183== by 0x55E1C6: Perl_runops_debug (dump.c:2450) ==20183== by 0x461B79: S_run_body (perl.c:2528) ==20183== by 0x46115C: perl_run (perl.c:2451) ==20183== by 0x41F1CD: main (perlmain.c:123) M dist/Storable/Storable.xs commit 3e998ddfb597cfae7bdb460b22e6c50440b1de92 Author: John Lightsey <[email protected]> Date: Tue Jan 24 10:30:18 2017 -0600 Fix stack buffer overflow in deserialization of hooks. The use of signed lengths resulted in a stack overflow in retrieve_hook() when a negative length was provided in the storable data. The retrieve_blessed() codepath had a similar problem with the placement of the trailing null byte when negative lengths were provided. M dist/Storable/Storable.pm M dist/Storable/Storable.xs M dist/Storable/t/store.t ----------------------------------------------------------------------- Summary of changes: dist/Storable/Storable.pm | 2 +- dist/Storable/Storable.xs | 20 ++++++++++++++------ dist/Storable/t/store.t | 12 +++++++++++- 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/dist/Storable/Storable.pm b/dist/Storable/Storable.pm index 397d584763..d8fd740eee 100644 --- a/dist/Storable/Storable.pm +++ b/dist/Storable/Storable.pm @@ -22,7 +22,7 @@ package Storable; @ISA = qw(Exporter); use vars qw($canonical $forgive_me $VERSION); -$VERSION = '2.61'; +$VERSION = '2.62'; BEGIN { if (eval { diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs index a72d84cbf5..9ba48be1c4 100644 --- a/dist/Storable/Storable.xs +++ b/dist/Storable/Storable.xs @@ -4048,7 +4048,7 @@ static SV *retrieve_idx_blessed(pTHX_ stcxt_t *cxt, const char *cname) */ static SV *retrieve_blessed(pTHX_ stcxt_t *cxt, const char *cname) { - I32 len; + U32 len; SV *sv; char buf[LG_BLESS + 1]; /* Avoid malloc() if possible */ char *classname = buf; @@ -4069,6 +4069,9 @@ static SV *retrieve_blessed(pTHX_ stcxt_t *cxt, const char *cname) if (len & 0x80) { RLEN(len); TRACEME(("** allocating %d bytes for class name", len+1)); + if (len > I32_MAX) { + CROAK(("Corrupted classname length")); + } New(10003, classname, len+1, char); malloced_classname = classname; } @@ -4119,7 +4122,7 @@ static SV *retrieve_blessed(pTHX_ stcxt_t *cxt, const char *cname) */ static SV *retrieve_hook(pTHX_ stcxt_t *cxt, const char *cname) { - I32 len; + U32 len; char buf[LG_BLESS + 1]; /* Avoid malloc() if possible */ char *classname = buf; unsigned int flags; @@ -4253,6 +4256,10 @@ static SV *retrieve_hook(pTHX_ stcxt_t *cxt, const char *cname) else GETMARK(len); + if (len > I32_MAX) { + CROAK(("Corrupted classname length")); + } + if (len > LG_BLESS) { TRACEME(("** allocating %d bytes for class name", len+1)); New(10003, classname, len+1, char); @@ -4274,6 +4281,11 @@ static SV *retrieve_hook(pTHX_ stcxt_t *cxt, const char *cname) TRACEME(("class name: %s", classname)); + if (!(flags & SHF_IDX_CLASSNAME) && classname != buf) { + /* some execution paths can throw an exception */ + SAVEFREEPV(classname); + } + /* * Decode user-frozen string length and read it in an SV. * @@ -4393,8 +4405,6 @@ static SV *retrieve_hook(pTHX_ stcxt_t *cxt, const char *cname) SEEN0_NN(sv, 0); SvRV_set(attached, NULL); SvREFCNT_dec(attached); - if (!(flags & SHF_IDX_CLASSNAME) && classname != buf) - Safefree(classname); return sv; } CROAK(("STORABLE_attach did not return a %s object", classname)); @@ -4475,8 +4485,6 @@ static SV *retrieve_hook(pTHX_ stcxt_t *cxt, const char *cname) SvREFCNT_dec(frozen); av_undef(av); sv_free((SV *) av); - if (!(flags & SHF_IDX_CLASSNAME) && classname != buf) - Safefree(classname); /* * If we had an <extra> type, then the object was not as simple, and diff --git a/dist/Storable/t/store.t b/dist/Storable/t/store.t index 3a4b9dca8a..b25dbd238f 100644 --- a/dist/Storable/t/store.t +++ b/dist/Storable/t/store.t @@ -19,7 +19,7 @@ sub BEGIN { use Storable qw(store retrieve store_fd nstore_fd fd_retrieve); -use Test::More tests => 24; +use Test::More tests => 25; $a = 'toto'; $b = \$a; @@ -101,5 +101,15 @@ isnt($@, ''); } } +{ + + my $frozen = + "\x70\x73\x74\x30\x04\x0a\x08\x31\x32\x33\x34\x35\x36\x37\x38\x04\x08\x08\x08\x03\xff\x00\x00\x00\x19\x08\xff\x00\x00\x00\x08\x08\xf9\x16\x16\x13\x16\x10\x10\x10\xff\x15\x16\x16\x16\x1e\x16\x16 ... [550 chars truncated] + open my $fh, '<', \$frozen; + eval { Storable::fd_retrieve($fh); }; + pass('RT 130635: no stack smashing error when retrieving hook'); + +} + close OUT or die "Could not close: $!"; END { 1 while unlink 'store' } -- Perl5 Master Repository
