Freeing $a or $b during sort causes a double free

[p5pRT]

Migrated from rt.perl.org#92264 (status was 'resolved')

Searchable as RT92264$

[p5pRT]

From @cpansprout

$ perl5.14.0 -e '@​_ = sort { *a = \1 } 1, 2'
Attempt to free unreferenced scalar​: SV 0x826480, Perl interpreter​: 0x800000.

I think pp_sort is misusing SAVESPTR.

---

Flags​:
  category=core
  severity=low
  ack=no

---

Site configuration information for perl 5.14.0​:

Configured by sprout at Wed May 11 13​:45​:58 PDT 2011.

Summary of my perl5 (revision 5 version 14 subversion 0) configuration​:
  Snapshot of​: eb70bb4
  Platform​:
  osname=darwin, osvers=10.5.0, archname=darwin-thread-multi-2level
  uname='darwin pint.local 10.5.0 darwin kernel version 10.5.0​: fri nov 5 23​:20​:39 pdt 2010; root​:xnu-1504.9.17~1release_i386 i386 '
  config_args='-Dusedevel -de -Duseithreads -Doptimize=-g'
  hint=recommended, useposix=true, d_sigaction=define
  useithreads=define, usemultiplicity=define
  useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
  use64bitint=undef, use64bitall=undef, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='cc', ccflags ='-fno-common -DPERL_DARWIN -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include',
  optimize='-g',
  cppflags='-fno-common -DPERL_DARWIN -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
  ccversion='', gccversion='4.2.1 (Apple Inc. build 5664)', gccosandvers=''
  intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
  d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
  ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
  alignbytes=8, prototype=define
  Linker and Libraries​:
  ld='env MACOSX_DEPLOYMENT_TARGET=10.3 cc', ldflags =' -fstack-protector -L/usr/local/lib'
  libpth=/usr/local/lib /usr/lib
  libs=-ldbm -ldl -lm -lutil -lc
  perllibs=-ldl -lm -lutil -lc
  libc=/usr/lib/libc.dylib, so=dylib, useshrplib=false, libperl=libperl.a
  gnulibc_version=''
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=bundle, d_dlsymun=undef, ccdlflags=' '
  cccdlflags=' ', lddlflags=' -bundle -undefined dynamic_lookup -L/usr/local/lib -fstack-protector'

Locally applied patches​:
  RC3

---

@​INC for perl 5.14.0​:
  /usr/local/lib/perl5/site_perl/5.14.0/darwin-thread-multi-2level
  /usr/local/lib/perl5/site_perl/5.14.0
  /usr/local/lib/perl5/5.14.0/darwin-thread-multi-2level
  /usr/local/lib/perl5/5.14.0
  /usr/local/lib/perl5/site_perl
  .

---

Environment for perl 5.14.0​:
  DYLD_LIBRARY_PATH (unset)
  HOME=/Users/sprout
  LANG=en_US.UTF-8
  LANGUAGE (unset)
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)
  PATH=/usr/bin​:/bin​:/usr/sbin​:/sbin​:/usr/local/bin​:/usr/X11/bin​:/usr/local/bin
  PERL_BADLANG (unset)
  SHELL=/bin/bash

[p5pRT]

From @cpansprout

On Sun Jun 05 15​:51​:34 2011, sprout wrote​:

$ perl5.14.0 -e '@​_ = sort { *a = \1 } 1, 2'
Attempt to free unreferenced scalar​: SV 0x826480, Perl interpreter​:
0x800000.

I think pp_sort is misusing SAVESPTR.

Attached is a test case. I don’t know how to fix this efficiently.
Currently GvSV(*a) and GvSV(*b) are not reference-counted during sort.
But no other code knows about that.

Making them reference-counted is the obvious fix, but I presume it was
not done originally for efficency’s sake.

--

Father Chrysostomos

[p5pRT]

From @cpansprout

Inline Patch

diff --git a/t/op/sort.t b/t/op/sort.t
index 6dedeeb..5911810 100644
--- a/t/op/sort.t
+++ b/t/op/sort.t
@@ -6,7 +6,7 @@ BEGIN {
     require 'test.pl';
 }
 use warnings;
-plan( tests => 171 );
+plan( tests => 172 );
 
 # these shouldn't hang
 {
@@ -960,3 +960,16 @@ is @x, 0, 'sort; returns empty list';
 eval '{@x = sort} 1';
 is $@, '', '{sort} does not die';
 is @x, 0, '{sort} returns empty list';
+
+# [perl #92264] Freeing $a or $b during sort
+{
+    my $w;
+    local $SIG{__WARN__} = sub { warn shift; ++ $w };
+    eval {
+      for (1,2) {
+	@x = sort { *a = \1; 1 } 1, 2;
+	@x = sort { *b = \1; 1 } 1, 2;
+      }
+    };
+    is $@, "", 'freeing $a or $b inside sort block';
+}

[p5pRT]

From [Unknown Contact. See original ticket]

On Sun Jun 05 15​:51​:34 2011, sprout wrote​:

$ perl5.14.0 -e '@​_ = sort { *a = \1 } 1, 2'
Attempt to free unreferenced scalar​: SV 0x826480, Perl interpreter​:
0x800000.

I think pp_sort is misusing SAVESPTR.

Attached is a test case. I don’t know how to fix this efficiently.
Currently GvSV(*a) and GvSV(*b) are not reference-counted during sort.
But no other code knows about that.

Making them reference-counted is the obvious fix, but I presume it was
not done originally for efficency’s sake.

--

Father Chrysostomos

[p5pRT]

@cpansprout - Status changed from 'new' to 'open'

[p5pRT]

From @cpansprout

On Sat Jan 14 22​:53​:05 2012, sprout wrote​:

On Sun Jun 05 15​:51​:34 2011, sprout wrote​:

$ perl5.14.0 -e '@​_ = sort { *a = \1 } 1, 2'
Attempt to free unreferenced scalar​: SV 0x826480, Perl interpreter​:
0x800000.

I think pp_sort is misusing SAVESPTR.

Attached is a test case. I don’t know how to fix this efficiently.
Currently GvSV(*a) and GvSV(*b) are not reference-counted during sort.
But no other code knows about that.

Making them reference-counted is the obvious fix, but I presume it was
not done originally for efficency’s sake.

But incrementing and decrementing a reference count should not be too
slow, and correctness is definitely a good thing. The attached patch is
the obvious fix, but it changes an existing TODO test into an assertion
failure.

So, this being rare enough, it should probably wait until the stack is
reference-counted.

--

Father Chrysostomos

[p5pRT]

From [Unknown Contact. See original ticket]

On Sat Jan 14 22​:53​:05 2012, sprout wrote​:

On Sun Jun 05 15​:51​:34 2011, sprout wrote​:

$ perl5.14.0 -e '@​_ = sort { *a = \1 } 1, 2'
Attempt to free unreferenced scalar​: SV 0x826480, Perl interpreter​:
0x800000.

I think pp_sort is misusing SAVESPTR.

Attached is a test case. I don’t know how to fix this efficiently.
Currently GvSV(*a) and GvSV(*b) are not reference-counted during sort.
But no other code knows about that.

Making them reference-counted is the obvious fix, but I presume it was
not done originally for efficency’s sake.

But incrementing and decrementing a reference count should not be too
slow, and correctness is definitely a good thing. The attached patch is
the obvious fix, but it changes an existing TODO test into an assertion
failure.

So, this being rare enough, it should probably wait until the stack is
reference-counted.

--

Father Chrysostomos

[p5pRT]

From zefram@fysh.org

The original test case on this ticket stopped producing the freeing
warning in commit 8b0c337 in 5.25.7.
That doesn't reflect any substantive difference relevant to this ticket.
This test case still warns​:

$ perl -le '() = sort { *a = \1 } 1, 2'
Attempt to free unreferenced scalar​: SV 0x1e4b270, Perl interpreter​: 0x1e24010.

-zefram

[p5pRT]

From zefram@fysh.org

This ticket has been misclassified as a stack-not-refcounted bug; it has
nothing to do with the stack. The comparands are on the stack, and if
this were a stack-not-refcounted bug then the issue would be the existence
of a comparand being undermined, causing it to be freed early and some
part of the operation seeing it freed. But in the supplied test case, the
comparands are in no danger of being undermined​: they're from literals,
so are referenced by const ops, and those references are not going away.

The issue is that the GvSV slot in globs *is* reference counted, but
sort wasn't respecting that reference counting. sort was writing to
GvSV without ever diddling reference counting. It got away with this as
long as nothing else wrote to GvSV of those globs during a sort, because
at the end it restored the original contents. But "*a = \1" performs a
refcount-respecting write to GvSV. This messes up the refcount of the
comparand, which gets decremented, *and* the refcount of the "1", which
gets incremented with the references leaked. By performing a larger
sort, with more instances of "*a = \1" executed, it's possible to move
refcounts the wrong way repeatedly​: the comparand can be subjected to
multiple attempts to free it when already freed, and the substitute $a
scalar can have its refcount go arbitrarily high from leaked refs.

Fixed in commit 16ada23.

-zefram

[p5pRT]

@xsawyerx - Status changed from 'open' to 'pending release'

[p5pRT]

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release yesterday of Perl 5.28.0, this and 185 other issues have been
resolved.

Perl 5.28.0 may be downloaded via​:
https://metacpan.org/release/XSAWYERX/perl-5.28.0

If you find that the problem persists, feel free to reopen this ticket.

[p5pRT]

@khwilliamson - Status changed from 'pending release' to 'resolved'