New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix potential segfault from pp_sort.c #16129
Comments
From @jplindermanAttached is what perlbug left in my mail spool. Mail cannot be sent from my |
From @jplindermanCreated by @jplindermanS_mergesortsv was saving the current comparison routine only when the The attached patch restores the pointer to the comparison routine For what it's worth, this bug has gone undetected because embedding a sort Perl Info
|
From @jkeenanOn Mon, 28 Aug 2017 22:41:29 GMT, jpl.jpl@gmail.com wrote:
In the years since the individual tests in lib/sort.t were created, it's become customary to produce test output that explicitly states that we did not have a crash or segfault where previously we would have had one. Hence, for the modifications for lib/sort.t I would prefer the '0002' patch I'm attaching to this RT. Since we prefer spaces to hard tabs in newly added code, I have also reformatted one test block. Please review. Thank you very much. |
From @jkeenan0001-Change-save-restore-behavior-for-comparisons.patchFrom 5fff7c19cd13984f53bc77b360fcdf90cd375551 Mon Sep 17 00:00:00 2001
From: jpl <jpl.jpl@gmail.com>
Date: Mon, 28 Aug 2017 09:54:15 -0400
Subject: [PATCH 1/2] Change save/restore behavior for comparisons
S_mergesortsv was saving the current comparison routine only when the
SORTf_DESC flag was set, but "restoring" it when ANY flag was set.
When some flag other than SORTf_DESC was set, this could lead to
the pointer to the comparison routine being set to NULL,
triggering a segfault when the routine was subsequently invoked.
---
lib/sort.t | 17 ++++++++++++++++-
pp_sort.c | 2 +-
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/lib/sort.t b/lib/sort.t
index b44269a..1ff3832 100644
--- a/lib/sort.t
+++ b/lib/sort.t
@@ -130,9 +130,24 @@ sub main {
}
}
-# Test with no pragma still loaded -- stability expected (this is a mergesort)
+# Test with no pragma yet loaded. Stability is expected from default sort.
main(sub { sort {&{$_[0]}} @{$_[1]} }, 0);
+# Verify that we have eliminated the segfault that could be triggered
+# by invoking a sort as part of a comparison routine.
+# No need for an explicit test. If we don't segfault, we're good.
+
+{
+ sub dumbsort {
+ my ($a, $b) = @_;
+ use sort qw( defaults stable );
+ my @ignore = sort (5,4,3,2,1);
+ return $a <=> $b;
+ }
+ use sort qw( defaults _qsort stable );
+ my @nested = sort { dumbsort($a,$b) } (3,2,2,1);
+}
+
{
use sort qw(_qsort);
my $sort_current; BEGIN { $sort_current = sort::current(); }
diff --git a/pp_sort.c b/pp_sort.c
index ee1dc5d..e0f4182 100644
--- a/pp_sort.c
+++ b/pp_sort.c
@@ -558,7 +558,7 @@ S_mergesortsv(pTHX_ gptr *base, size_t nmemb, SVCOMPARE_t cmp, U32 flags)
}
done:
if (aux != small) Safefree(aux); /* free iff allocated */
- if (flags) {
+ if (savecmp != NULL) {
PL_sort_RealCmp = savecmp; /* Restore current comparison routine, if any */
}
return;
--
2.7.4
|
From @jkeenan0002-Make-explicit-that-we-did-not-segfault.patchFrom a97b249566872bfc9f297c723d08f847e696eabf Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Mon, 28 Aug 2017 21:57:28 -0400
Subject: [PATCH 2/2] Make explicit that we did not segfault.
Do not use hard tabs in newly added test blocks.
---
lib/sort.t | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/lib/sort.t b/lib/sort.t
index 1ff3832..8688bb2 100644
--- a/lib/sort.t
+++ b/lib/sort.t
@@ -28,6 +28,7 @@ use warnings;
use Test::More tests => @TestSizes * 2 # sort() tests
* 6 # number of pragmas to test
+ 1 # extra test for qsort instability
+ + 1 # extra test to demonstrate no segfault
+ 3 # tests for sort::current
+ 3; # tests for "defaults" and "no sort"
@@ -135,17 +136,17 @@ main(sub { sort {&{$_[0]}} @{$_[1]} }, 0);
# Verify that we have eliminated the segfault that could be triggered
# by invoking a sort as part of a comparison routine.
-# No need for an explicit test. If we don't segfault, we're good.
{
sub dumbsort {
- my ($a, $b) = @_;
- use sort qw( defaults stable );
- my @ignore = sort (5,4,3,2,1);
- return $a <=> $b;
+ my ($a, $b) = @_;
+ use sort qw( defaults stable );
+ my @ignore = sort (5,4,3,2,1);
+ return $a <=> $b;
}
use sort qw( defaults _qsort stable );
my @nested = sort { dumbsort($a,$b) } (3,2,2,1);
+ pass("No segfault when sort invoked as part of comparison routine: RT #131984");
}
{
--
2.7.4
|
The RT System itself - Status changed from 'new' to 'open' |
From @jkeenanOn Tue, 29 Aug 2017 02:06:28 GMT, jkeenan wrote:
Available for smoking in branch: -- |
From @tonycozOn Mon, 28 Aug 2017 15:41:29 -0700, jpl.jpl@gmail.com wrote:
The patch makes sense to me. I do suspect both before and after your change there's an issue if an inner sort dies and is caught within the outer sort. Tony |
From @jplindermanThe changes look good. I'll try to remember to use blanks instead of tabs On Mon, Aug 28, 2017 at 10:06 PM, James E Keenan via RT <
|
From @jplindermanI agree, Tony, but I don't know what to do about it. The saving grace, I On Tue, Aug 29, 2017 at 12:52 AM, Tony Cook via RT <
|
From @hvdsOn Tue, 29 Aug 2017 04:16:28 -0700, jpl.jpl@gmail.com wrote:
Done indirectly, it's not so surprising - { $a->method <=> $b->method } is a perfectly reasonable comparator, and the method may do all sorts of things. Hugo |
From @jplindermanThe method can do all sorts😀 of things, but it must always return On Tue, Aug 29, 2017 at 3:05 PM, Hugo van der Sanden via RT <
|
From @jplindermanHere's a case that I find plausible. You have an array of hashrefs, each On Tue, Aug 29, 2017 at 3:05 PM, Hugo van der Sanden via RT <
|
From zefram@fysh.orgJohn P. Linderman wrote:
{ join("\0", sort $a->flanges) cmp join("\0", sort $b->flanges) } -zefram |
From @jplindermanIs this commit waiting for something from me? I see that other commits to commit f6107ca I'd like to continue work on destabilization, but I'd like to start from a On Mon, Aug 28, 2017 at 10:06 PM, James E Keenan via RT <
|
From @jplindermanThere's a Draconian, but I think effective, workaround. Assign the original On Tue, Aug 29, 2017 at 12:52 AM, Tony Cook via RT <
|
From @tonycozOn Sun, Sep 10, 2017 at 09:53:58AM -0400, John P. Linderman wrote:
The general solution for this type of thing in core is usually a So we'd need a new macro, code in scope.c to handle it, and a call to it Tony |
From @tonycozOn Mon, 04 Sep 2017 08:03:29 -0700, jpl.jpl@gmail.com wrote:
Thanks, applied as 0e1d050. Leaving open for now, since we still have a known issue. Tony |
@tonycoz, could you review this ticket and clarify the "known issue"? It might make sense to open a new GH issue for that. Thank you very much. |
This makes special-cased forms such as sort { $a <=> $b } even faster. Also, since this commit removes PL_sort_RealCmp, it fixes the issue with nested sort calls mentioned in gh Perl#16129
This makes special-cased forms such as sort { $b <=> $a } even faster. Also, since this commit removes PL_sort_RealCmp, it fixes the issue with nested sort calls mentioned in gh Perl#16129
This makes special-cased forms such as sort { $b <=> $a } even faster. Also, since this commit removes PL_sort_RealCmp, it fixes the issue with nested sort calls mentioned in gh Perl#16129
I'll copy/paste a related snippet, although you probably already know about
it. It went to perlbug-followup. -- jpl
On Sun, Sep 10, 2017 at 09:53:58AM -0400, John P. Linderman wrote:
On Tue, Aug 29, 2017 at 12:52 AM, Tony Cook via RT <
***@***.***> wrote:
> On Mon, 28 Aug 2017 15:41:29 -0700, ***@***.*** wrote:
> > Attached is what perlbug left in my mail spool. Mail cannot be sent
from
> my
> > PC.
>
>
> The patch makes sense to me.
>
> I do suspect both before and after your change there's an issue if an
> inner sort dies and is caught within the outer sort.
>
> Tony
>
There's a Draconian, but I think effective, workaround. Assign the
original
comparison routine to PL_sort_RealCmp before *every* comparison. That's
NlogN assignments rather than 1, but it's probably insignificant relative
to all the other stuff that happens around comparisons. This makes one
appreciate how painful it is that C does not support closures. The authors
of *go* didn't repeat that oversight.
The general solution for this type of thing in core is usually a
SAVE*() macro such as SAVEPPTR() (see scope.h, scope.c), but function
pointers are generally not compatible with data pointers.
So we'd need a new macro, code in scope.c to handle it, and a call to it
in pp_sort.c
Tony
…On Mon, Mar 2, 2020 at 4:00 PM James E Keenan ***@***.***> wrote:
From @tonycoz <https://github.com/tonycoz>
On Mon, 04 Sep 2017 08:03:29 -0700, jpl.jpl@gmail.com wrote:
Is this commit waiting for something from me? I see that other commits
to
pp_sort.c have been made:
commit f6107ca
<f6107ca>
I'd like to continue work on destabilization, but I'd like to start
from a
point where the segfault problem has been addressed.
Thanks, applied as 0e1d050
<0e1d050>
.
Leaving open for now, since we still have a known issue.
Tony
@tonycoz <https://github.com/tonycoz>, could you review this ticket and
clarify the "known issue"?
It might make sense to open a new GH issue for that.
@jplinderman <https://github.com/jplinderman>
Thank you very much.
Jim Keenan
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16129?email_source=notifications&email_token=AAAYPV5PPVC5KZMJ2C72VO3RFQNAHA5CNFSM4K74PYOKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENQ6SNY#issuecomment-593619255>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAYPV3FJWGJ2Y62KUWJBCTRFQNAHANCNFSM4K74PYOA>
.
|
perlbug-followup no longer works, unfortunately (though it possibly did on that date). |
This makes special-cased forms such as sort { $b <=> $a } even faster. Also, since this commit removes PL_sort_RealCmp, it fixes the issue with nested sort calls mentioned in gh Perl#16129
This makes special-cased forms such as sort { $b <=> $a } even faster. Also, since this commit removes PL_sort_RealCmp, it fixes the issue with nested sort calls mentioned in gh Perl#16129
This makes special-cased forms such as sort { $b <=> $a } even faster. Also, since this commit removes PL_sort_RealCmp, it fixes the issue with nested sort calls mentioned in gh Perl#16129
This makes special-cased forms such as sort { $b <=> $a } even faster. Also, since this commit removes PL_sort_RealCmp, it fixes the issue with nested sort calls mentioned in gh Perl#16129
This makes special-cased forms such as sort { $b <=> $a } even faster. Also, since this commit removes PL_sort_RealCmp, it fixes the issue with nested sort calls mentioned in gh Perl#16129
This makes special-cased forms such as sort { $b <=> $a } even faster. Also, since this commit removes PL_sort_RealCmp, it fixes the issue with nested sort calls mentioned in gh Perl#16129
This makes special-cased forms such as sort { $b <=> $a } even faster. Also, since this commit removes PL_sort_RealCmp, it fixes the issue with nested sort calls mentioned in gh Perl#16129
This makes special-cased forms such as sort { $b <=> $a } even faster. Also, since this commit removes PL_sort_RealCmp, it fixes the issue with nested sort calls mentioned in gh #16129
|
Migrated from rt.perl.org#131984 (status was 'open')
Searchable as RT131984$
The text was updated successfully, but these errors were encountered: