develooper Front page | perl.perl5.porters | Postings from February 2018

Re: [perl #131046] [PATCH] Carp: Do not crash when reading @DB::args

Thread Previous | Thread Next
February 24, 2018 12:33
Re: [perl #131046] [PATCH] Carp: Do not crash when reading @DB::args
Message ID:
On 24 February 2018 at 01:00, Zefram <> wrote:
> demerphq wrote:
>>Patch pushed as 4764858cb80e76fdba33cc1b3be8fcdef26df754
> There are a couple of problems with this patch, going beyond my
> distaste for the inadequacy of the code change.  Firstly, the commit
> message contains exactly the kind of misconception that I argued against.
> Rather than describing the effect as partial, the message says "This patch
> safely iterates all elements", "This prevent crashing perl", and "It [is]
> ... workaround for Carp module".  (Bad grammar all in the actual message.)
> This is describing the patch as having an effectiveness that it doesn't.

Ok, so, I have pushed:


Which I believe documents your concerns here in both the code and the
commit message.

> Secondly, the newly added test is predicated on that very same mythical
> effectiveness.  It sets up a stack refcounting failure, at which point
> behaviour is undefined, whether the Carp patch is applied or not.
> This is not a correct test: it can fail without anything getting more
> wrong than it already is.

We can debate this part now. ;-)

Do you really think this test should be removed? It seems a reasonable
thing to test given the intent of this patch series.

> The last argument advanced in favour of this patch, unlike many that
> had preceded it, was supposedly based on a correct understanding of its
> limitations.  I didn't argue against that, because my greater objection
> is about the proposed patch being misunderstood and oversold.  Where it's
> correctly understood, it's more a matter of opinion whether it's useful.
> But the patch as applied doesn't seem to have taken that correct
> understanding into account.  It is firmly based on the misconception,
> and I seem to have been fooled into dropping my objections by some talk
> that doesn't actually reflect the totality of the patch.

I really hope that the commentary I added satisfies your objections here.

I hope you understand that I had no intention to fool you in any way,
(and why you suggesting such might have annoyed me in previous

> Bonus problem: the patch desynchronises the version numbers of Carp and
> Carp::Heavy, giving the former an unjustified underscored version.

/This/ point we need to debate. :-)

I believe that every distinct code state of a module in dist/ should
have a distinct version number.

I also believe that minor changes in dist modules during the lifetime
of a dev version should be marked as a "dev" versions, and that only
prior to release should we bump them to a "non-dev" version. I believe
that others have felt the same way in at least some cases as Carp has
code to defend against dev/underbar versions.

Related to this, I have bumped the versions twice now, meaning two
conceptually related changes have resulted in a jump from 1.45 to
1.48. Personally I think this is silly as it means there are version
gaps in the CPAN published copies of this code, for instance as far as
I can tell the latest version on CPAN is 1.38.

> Clearly the new test needs to be deleted.

I really don't understand why, and have not done so myself.

> I don't know what to do about
> the false commit message, though.  The message doesn't reference this
> ticket, so putting a correction here doesn't serve as an annotation.
> I'm tempted to revert the commit, on the basis that the code change can
> later be applied again with a correct message.  Then at least git blame
> would point at something correct.

Given the code churn in Carp this was no longer an option, or at least
was a bad option.

I elected to push 02c84d7f0f97e083f5d8ea9856488f3ede09364f as a
compromise. (See below)

I hope with the sequence of changes here you feel that your concerns
have been adequately addressed.


commit 02c84d7f0f97e083f5d8ea9856488f3ede09364f
Author: Yves Orton <>
Date:   Sat Feb 24 12:48:46 2018 +0100

    Carp: add comment explaining the fix for perl #131046

    In 4764858cb80e76fdba33cc1b3be8fcdef26df754 I added code that
    eval's the argument extraction from DB::args() (code written by
    both Pali and myself independently).

    Unfortunately the commit message for that commit was somewhat
    misleading, making it sound like the patch was a complete fix for
    the underlying problem of stack-not-refcounted bugs, when in fact
    it is actually a workaround, and a not-universally popular one
    either due to its incompleteness. For more details of why it is
    not popular see Zeframs commentary in

    Despite the concerns expressed by Zefram when considered from the
    POV of a large enterprise using Carp to trap exceptions from
    application code, having Carp throw its own exceptions about
    stack-not-refcounted bugs and thus hiding the applications error
    data is a significant issue.

    So in 4764858cb80e76fdba33cc1b3be8fcdef26df754 we use eval to
    handle those cases where Perl /can/ detect a stack-not-refcounted
    error, and thus preserve as much of the applications error data
    as possible.

    This commit adds a comment to Carp which spells all of this out.

diff --git a/dist/Carp/Changes b/dist/Carp/Changes
index db39203..2b549d9 100644
--- a/dist/Carp/Changes
+++ b/dist/Carp/Changes
@@ -1,3 +1,9 @@
+version 1.49
+ * comment only change, document the change from 1.47 better
+   and create a commit in blead-perl which can be used to
+   reference this issue from the bug report.
 version 1.48

  * guard against hand-rolled UNIVERSAL::can() implementations
diff --git a/dist/Carp/lib/ b/dist/Carp/lib/
index 9956f12..419ba9c 100644
--- a/dist/Carp/lib/
+++ b/dist/Carp/lib/
@@ -116,7 +116,7 @@ BEGIN {

-our $VERSION = '1.48';
+our $VERSION = '1.49';
 $VERSION =~ tr/_//d;

 our $MaxEvalLen = 0;
@@ -232,7 +232,18 @@ sub caller_info {

     my $sub_name = Carp::get_subname( \%call_info );
     if ( $call_info{has_args} ) {
-        # guard our serialization of the stack from stack refcounting bugs
+        # Guard our serialization of the stack from stack refcounting bugs
+        # NOTE this is NOT a complete solution, we cannot 100% guard against
+        # these bugs.  However in many cases Perl *is* capable of detecting
+        # them and throws an error when it does.  Unfortunately serializing
+        # the arguments on the stack is a perfect way of finding these bugs,
+        # even when they would not affect normal program flow that did not
+        # poke around inside the stack.  Inside of it makes little
+        # sense reporting these bugs, as Carp's job is to report the callers
+        # errors, not the ones it might happen to tickle while doing so.
+        # See:
+        # and:
+        # for more details and discussion. - Yves
         my @args = map {
                 my $arg;
                 local $@= $@;
diff --git a/dist/Carp/lib/Carp/ b/dist/Carp/lib/Carp/
index 5188f40..e2b7292 100644
--- a/dist/Carp/lib/Carp/
+++ b/dist/Carp/lib/Carp/
@@ -2,7 +2,7 @@ package Carp::Heavy;

 use Carp ();

-our $VERSION = '1.48';
+our $VERSION = '1.49';
 $VERSION =~ tr/_//d;

 # Carp::Heavy was merged into Carp in version 1.12.  Any mismatched versions

perl -Mre=debug -e "/just|another|perl|hacker/"

Thread Previous | Thread Next Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at | Group listing | About