From d9c4b4ae5a1a17347ff5e3ecbf8e1d9da481f476 Mon Sep 17 00:00:00 2001 From: David Mitchell Date: Wed, 3 Apr 2019 13:23:24 +0100 Subject: [PATCH] Data::Dumper - avoid leak on croak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v5.21.3-742-g19be3be696 added a facility to Dumper.xs to croak if the recursion level became too deep (1000 by default). The trouble with this is that various parts of DD_dump() allocate temporary SVs and buffers, which will leak if DD_dump() unceremoniously just croaks(). This currently manifests as dist/Data-Dumper/t/recurse.t failing under Address Sanitiser. This commit makes the depth checking code just set a sticky 'too deep' boolean flag, and a) on entry, DD_dump() just returns immediately if the flag is set; b) the flag is checked by the top-level called of DD_dump() and croaks if set. So the net effect is to defer croaking until the dump is complete, and avoid any further recursion once the flag is set. This is a bit of a quick fix. More long-term solutions would be to convert DD_dump() to be iterative rather than recursive, and/or make sure all temporary SVs and buffers are suitably anchored somewhere so that they get cleaned up on croak. Petr Písař: Ported from 6d65cb5d847ac93680949c4fa02111808207fbdc in perl git tree. Signed-off-by: Petr Písař --- Dumper.pm | 6 +++--- Dumper.xs | 27 ++++++++++++++++++++------- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/Dumper.pm b/Dumper.pm index 40aeb7d..06af4c4 100644 --- a/Dumper.pm +++ b/Dumper.pm @@ -10,7 +10,7 @@ package Data::Dumper; BEGIN { - $VERSION = '2.173'; # Don't forget to set version and release + $VERSION = '2.174'; # Don't forget to set version and release } # date in POD below! #$| = 1; @@ -1461,13 +1461,13 @@ be to use the C filter of Data::Dumper. Gurusamy Sarathy gsar@activestate.com -Copyright (c) 1996-2017 Gurusamy Sarathy. All rights reserved. +Copyright (c) 1996-2019 Gurusamy Sarathy. All rights reserved. This program is free software; you can redistribute it and/or modify it under the same terms as Perl itself. =head1 VERSION -Version 2.173 +Version 2.174 =head1 SEE ALSO diff --git a/Dumper.xs b/Dumper.xs index 7f0b027..a324cb6 100644 --- a/Dumper.xs +++ b/Dumper.xs @@ -61,9 +61,10 @@ #endif /* This struct contains almost all the user's desired configuration, and it - * is treated as constant by the recursive function. This arrangement has - * the advantage of needing less memory than passing all of them on the - * stack all the time (as was the case in an earlier implementation). */ + * is treated as mostly constant (except for maxrecursed) by the recursive + * function. This arrangement has the advantage of needing less memory + * than passing all of them on the stack all the time (as was the case in + * an earlier implementation). */ typedef struct { SV *pad; SV *xpad; @@ -74,6 +75,7 @@ typedef struct { SV *toaster; SV *bless; IV maxrecurse; + bool maxrecursed; /* at some point we exceeded the maximum recursion level */ I32 indent; I32 purity; I32 deepcopy; @@ -97,7 +99,7 @@ static bool safe_decimal_number(const char *p, STRLEN len); static SV *sv_x (pTHX_ SV *sv, const char *str, STRLEN len, I32 n); static I32 DD_dump (pTHX_ SV *val, const char *name, STRLEN namelen, SV *retval, HV *seenhv, AV *postav, const I32 level, SV *apad, - const Style *style); + Style *style); #ifndef HvNAME_get #define HvNAME_get HvNAME @@ -615,7 +617,7 @@ deparsed_output(pTHX_ SV *val) */ static I32 DD_dump(pTHX_ SV *val, const char *name, STRLEN namelen, SV *retval, HV *seenhv, - AV *postav, const I32 level, SV *apad, const Style *style) + AV *postav, const I32 level, SV *apad, Style *style) { char tmpbuf[128]; Size_t i; @@ -642,6 +644,9 @@ DD_dump(pTHX_ SV *val, const char *name, STRLEN namelen, SV *retval, HV *seenhv, if (!val) return 0; + if (style->maxrecursed) + return 0; + /* If the output buffer has less than some arbitrary amount of space remaining, then enlarge it. For the test case (25M of output), *1.1 was slower, *2.0 was the same, so the first guess of 1.5 is @@ -793,7 +798,7 @@ DD_dump(pTHX_ SV *val, const char *name, STRLEN namelen, SV *retval, HV *seenhv, } if (style->maxrecurse > 0 && level >= style->maxrecurse) { - croak("Recursion limit of %" IVdf " exceeded", style->maxrecurse); + style->maxrecursed = TRUE; } if (realpack && !no_bless) { /* we have a blessed ref */ @@ -1528,6 +1533,7 @@ Data_Dumper_Dumpxs(href, ...) style.indent = 2; style.quotekeys = 1; style.maxrecurse = 1000; + style.maxrecursed = FALSE; style.purity = style.deepcopy = style.useqq = style.maxdepth = style.use_sparse_seen_hash = style.trailingcomma = 0; style.pad = style.xpad = style.sep = style.pair = style.sortkeys @@ -1675,7 +1681,7 @@ Data_Dumper_Dumpxs(href, ...) DD_dump(aTHX_ val, SvPVX_const(name), SvCUR(name), valstr, seenhv, postav, 0, newapad, &style); SPAGAIN; - + if (style.indent >= 2 && !terse) SvREFCNT_dec(newapad); @@ -1715,6 +1721,13 @@ Data_Dumper_Dumpxs(href, ...) } SvREFCNT_dec(postav); SvREFCNT_dec(valstr); + + /* we defer croaking until here so that temporary SVs and + * buffers won't be leaked */ + if (style.maxrecursed) + croak("Recursion limit of %" IVdf " exceeded", + style.maxrecurse); + } else croak("Call to new() method failed to return HASH ref"); -- 2.20.1