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
From:
demerphq
Date:
February 24, 2018 12:33
Subject:
Re: [perl #131046] [PATCH] Carp: Do not crash when reading @DB::args
Message ID:
CANgJU+U1+J8YSw_qCfgE4B52Q=nUJ_=kHw6nEG3XV9myyWc9og@mail.gmail.com
On 24 February 2018 at 01:00, Zefram <zefram@fysh.org> 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:

02c84d7f0f97e083f5d8ea9856488f3ede09364f

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
replies.)

>
> 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.

Cheers,
Yves

commit 02c84d7f0f97e083f5d8ea9856488f3ede09364f
Author: Yves Orton <demerphq@gmail.com>
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
    https://rt.perl.org/Public/Bug/Display.html?id=131046

    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/Carp.pm b/dist/Carp/lib/Carp.pm
index 9956f12..419ba9c 100644
--- a/dist/Carp/lib/Carp.pm
+++ b/dist/Carp/lib/Carp.pm
@@ -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 Carp.pm 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: https://rt.perl.org/Public/Bug/Display.html?id=131046
+        # and: https://rt.perl.org/Public/Bug/Display.html?id=52610
+        # for more details and discussion. - Yves
         my @args = map {
                 my $arg;
                 local $@= $@;
diff --git a/dist/Carp/lib/Carp/Heavy.pm b/dist/Carp/lib/Carp/Heavy.pm
index 5188f40..e2b7292 100644
--- a/dist/Carp/lib/Carp/Heavy.pm
+++ b/dist/Carp/lib/Carp/Heavy.pm
@@ -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


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About