Skip to content
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

Attempt to free unreferenced scalar (in 49 lines) #344

Closed
p5pRT opened this issue Aug 5, 1999 · 7 comments
Closed

Attempt to free unreferenced scalar (in 49 lines) #344

p5pRT opened this issue Aug 5, 1999 · 7 comments

Comments

@p5pRT
Copy link

p5pRT commented Aug 5, 1999

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

Searchable as RT1176$

@p5pRT
Copy link
Author

p5pRT commented Aug 5, 1999

From @nwc10

I've got this as short as possible. On Solaris the script attached causes

Attempt to free unreferenced scalar at crashit line 29, <DATA> chunk 3.

for perl5.00502, or a coredump on 5.00503, 5.00557, 5.00558 and 5.00560.
(gcc compiled), and 3 "attempt to free unreferenced scalar"s on 5.00560
(Sun CC v5, 64 bit)

The coredump is in scope.c
S_save_scalar_at
called from Perl_save_aelem

at the line

SvMAGIC(sv) = SvMAGIC(osv);

which is because osv is an SV

{sv_any = 0x17ae78, sv_refcnt = 2, sv_flags = 255}

and has garbage in the xmg_magic pointer (0x200) which SEGVs when deferenced

I've used a watchpoint to track what happened to this SV at osv, and last
thing that's happened to it is that it's been freed at

#0 0xc3a90 in Perl_sv_free (sv=0x17ae60) at sv.c​:3170
#1 0xa7280 in Perl_av_clear (av=0x188350) at av.c​:352
#2 0xb5bc4 in Perl_pp_entersub () at pp_hot.c​:2377
#3 0xa995c in Perl_runops_debug () at run.c​:57
#4 0x295cc in S_run_body (args=0xffbefb60) at perl.c​:1070
#5 0xec9dc in Perl_vdefault_protect (excpt=0xffbefb78,
  body=0x29364 <S_run_body>, args=0xffbefaf8) at scope.c​:44
#6 0xec8e8 in Perl_default_protect (excpt=0xffbefb78,
  body=0x29364 <S_run_body>) at scope.c​:25
#7 0x29024 in perl_run (my_perl=0x16e008) at perl.c​:1003
#8 0x26600 in main (argc=3, argv=0xffbefc6c, env=0xffbefc7c)
  at miniperlmain.c​:53

The script is a paired down HTML (plus other stuff) parser, and the bug goes
away if I comment out two or more of the local statements.

I can probably work around the problem, but it would be nice to blat the bug.
IIRC getting shot of the the "Attempt to free unreferenced scalar" bug(s) is
tricky because they are very hard to tie down.

I've just tried the script on BSDI and doesn't crash. Hmm. Maybe it's Solaris
specific.

Nicholas Clark
------------------------------------------------------------------------------
#!/home/nick/perldev/bin/perl -w
use strict;

sub tag {
  # OK, when we leave this scope we restore all these nesting things
  local $_[3];
  local $_[4];
  local $_[5];
  # Doing this as $_[1] to affect its pos()
  $_[1] =~ m!\G(\!?\w+) # The tag itself
  !gxc;
 
  my ($argtext) = tag_args (\&tag_args, @​_[1..5], '');
}

# input ($coderef,$text,$inpre,$ifnest,$varnest,$tagnest)
# return ($text, $tag)
sub tag_args {
  while ($_[1] =~ m!([<>]?)!gxc) {
  return if ($1 eq '>')
  }
}

# input ($coderef,$text,$inpre,$ifnest,$varnest,$tagnest,$coderef)
# return ($text, $tag)
sub parse_html {
  while (pos ($_[1]) < length $_[1]) {
  if ($_[1] =~ /\G</gc) {
  # tag
  my ($opentag, $tag_type) = tag (\&amp;parse_html, @​_[1..$#_]);
  } elsif ($_[1] =~ /\G([^<]+)/gc) {
  }
  }
  return '';
}

while (<DATA>) {
  my $input = $_;
  pos ($input) = 0;
  my ($inpre, $ifnest, $varnest, $tagnest);
  () = parse_html (undef, $input, $inpre, $ifnest, $varnest,$tagnest);
}

__DATA__
<HTML>
<HEAD>
<TITLE>Nick's own boohoo
------------------------------------------------------------------------------

Perl Info


Site configuration information for perl 5.00502:

Summary of my perl5 (5.0 patchlevel 5 subversion 2) configuration:
  Platform:
    osname=solaris, osvers=2.7, archname=sun4-solaris
    uname='sunos 5.7 s998_18 sun4u sparc sunw,ultra-1 '
    hint=previous, useposix=true, d_sigaction=define
    usethreads=undef useperlio=undef d_sfio=undef
  Compiler:
    cc='gcc', optimize='-O', gccversion=2.8.1
    cppflags='-I/usr/local/include'
    ccflags ='-I/usr/local/include'
    stdchar='char', d_stdstdio=define, usevfork=false
    intsize=4, longsize=4, ptrsize=4, doublesize=8
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    alignbytes=8, usemymalloc=y, prototype=define
  Linker and Libraries:
    ld='gcc', ldflags =' -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib /usr/ccs/lib
    libs=-lsocket -lnsl -lgdbm -ldl -lm -lc -lcrypt
    libc=/lib/libc.so, so=so, useshrplib=false, libperl=libperl.a
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags=' '
    cccdlflags='-fPIC', lddlflags='-G -L/usr/local/lib'

Locally applied patches:
    


@INC for perl 5.00502:
    /usr/local/lib/perl5/5.00502/sun4-solaris
    /usr/local/lib/perl5/5.00502
    /usr/local/lib/perl5/site_perl/5.005/sun4-solaris
    /usr/local/lib/perl5/site_perl/5.005
    .


Environment for perl 5.00502:
    HOME=/home/nick
    LANG=en_GB
    LC_COLLATE=en_GB
    LC_CTYPE=en_GB
    LC_MESSAGES=C
    LC_MONETARY=en_GB
    LC_NUMERIC=en_GB
    LC_TIME=en_GB
    LD_LIBRARY_PATH=/home/nick/lib/:/home/nick/bin/:/usr/local/lib:/home/nick/lib/:/home/nick/bin/:/usr/local/lib:
    LOGDIR (unset)
    PATH=/home/nick/bin:/usr/local/bin:/home/nick/bin:/usr/local/bin:/usr/dt/bin:/usr/openwin/bin:/bin:/usr/bin:/usr/ucb:/usr/openwin/bin:/usr/ccs/bin:/sbin:/usr/sbin:/usr/local/sbin:/usr/etc:/home/scot/bin:/usr/ccs/bin:/sbin:/usr/sbin:/usr/local/sbin:/usr/etc:/home/scot/bin
    PERL_BADLANG (unset)
    SHELL=/usr/local/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Jul 18, 2002

From @hvds

[Too late for 5.8.0, sorry.]

Nicholas Clark once wrote​:
:I've got this as short as possible.

<giggle>

This script​:
  perl -we 'sub f { local $_[0] } f()'
coredumps in leave_scope, because (to try and take advantage of a goto)
it frees the av before restoring the sv into it.

Fixing that still leaves the 'attempt to free' problems in the original
code, which can be reduced to​:
  crypt% ./perl -we 'sub a { local($_[1],$_[2],$_[3]) } sub b { $x++ && a(0..2) for 1..2 } b() for 1..2'
  Name "main​::x" used only once​: possible typo at -e line 1.
  Attempt to free unreferenced scalar at -e line 1.
  Attempt to free temp prematurely​: SV 0x814b288 at -e line 1.
  Attempt to free unreferenced scalar at -e line 1.
  crypt%
.... which I haven't yet attempted to investigate.

I note that this test case still coredumps when run with '-Ds', though
it gets further than without the patch below. I suspect that's
a separate problem, probably due to bitrot in the rarely used -Ds code.

The patch below breaks out the code it wants to goto, so that we can
conveniently call it before the SvREFCNT_dec(av), and fixes up similar
problems in parallel branches.

This patch requires 'make regen_headers'.

Hugo

Inline Patch
--- embed.fnc.old	Tue Jul 16 05:02:57 2002
+++ embed.fnc	Thu Jul 18 15:30:20 2002
@@ -1190,6 +1190,7 @@
 #endif
 
 #if defined(PERL_IN_SCOPE_C) || defined(PERL_DECL_PROT)
+s	|void	|restore_sv	|SV *value|SV **ptr
 s	|SV*	|save_scalar_at	|SV **sptr
 #endif
 
--- scope.c.old	Sun May 19 02:32:24 2002
+++ scope.c	Thu Jul 18 15:32:35 2002
@@ -654,6 +654,43 @@
     return start;
 }
 
+void S_restore_sv(SV *value, SV **ptr) {
+    SV *sv = *ptr;
+    DEBUG_S(PerlIO_printf(Perl_debug_log,
+			  "restore svref: %p %p:%s -> %p:%s\n",
+			  ptr, sv, SvPEEK(sv), value, SvPEEK(value)));
+    if (SvTYPE(sv) >= SVt_PVMG && SvMAGIC(sv) &&
+	SvTYPE(sv) != SVt_PVGV)
+    {
+	(void)SvUPGRADE(value, SvTYPE(sv));
+	SvMAGIC(value) = SvMAGIC(sv);
+	SvFLAGS(value) |= SvMAGICAL(sv);
+	SvMAGICAL_off(sv);
+	SvMAGIC(sv) = 0;
+    }
+    /* XXX This branch is pretty bogus.  This code irretrievably
+     * clears(!) the magic on the SV (either to avoid further
+     * croaking that might ensue when the SvSETMAGIC() below is
+     * called, or to avoid two different SVs pointing at the same
+     * SvMAGIC()).  This needs a total rethink.  --GSAR */
+    else if (SvTYPE(value) >= SVt_PVMG && SvMAGIC(value) &&
+	     SvTYPE(value) != SVt_PVGV)
+    {
+	SvFLAGS(value) |= (SvFLAGS(value) &
+			  (SVp_NOK|SVp_POK)) >> PRIVSHIFT;
+	SvMAGICAL_off(value);
+	/* XXX this is a leak when we get here because the
+	 * mg_get() in save_scalar_at() croaked */
+	SvMAGIC(value) = 0;
+    }
+    SvREFCNT_dec(sv);
+    *ptr = value;
+    PL_localizing = 2;
+    SvSETMAGIC(value);
+    PL_localizing = 0;
+    SvREFCNT_dec(value);
+}
+
 void
 Perl_leave_scope(pTHX_ I32 base)
 {
@@ -681,9 +718,9 @@
 	case SAVEt_SV:				/* scalar reference */
 	    value = (SV*)SSPOPPTR;
 	    gv = (GV*)SSPOPPTR;
-	    ptr = &GvSV(gv);
+	    restore_sv(value, (SV**)&GvSV(gv));
 	    SvREFCNT_dec(gv);
-	    goto restore_sv;
+	    break;
 	case SAVEt_GENERIC_PVREF:		/* generic pv */
 	    str = (char*)SSPOPPTR;
 	    ptr = SSPOPPTR;
@@ -714,42 +751,7 @@
 	    break;
 	case SAVEt_SVREF:			/* scalar reference */
 	    value = (SV*)SSPOPPTR;
-	    ptr = SSPOPPTR;
-	restore_sv:
-	    sv = *(SV**)ptr;
-	    DEBUG_S(PerlIO_printf(Perl_debug_log,
-				  "restore svref: %p %p:%s -> %p:%s\n",
-				  ptr, sv, SvPEEK(sv), value, SvPEEK(value)));
-	    if (SvTYPE(sv) >= SVt_PVMG && SvMAGIC(sv) &&
-		SvTYPE(sv) != SVt_PVGV)
-	    {
-		(void)SvUPGRADE(value, SvTYPE(sv));
-		SvMAGIC(value) = SvMAGIC(sv);
-		SvFLAGS(value) |= SvMAGICAL(sv);
-		SvMAGICAL_off(sv);
-		SvMAGIC(sv) = 0;
-	    }
-	    /* XXX This branch is pretty bogus.  This code irretrievably
-	     * clears(!) the magic on the SV (either to avoid further
-	     * croaking that might ensue when the SvSETMAGIC() below is
-	     * called, or to avoid two different SVs pointing at the same
-	     * SvMAGIC()).  This needs a total rethink.  --GSAR */
-	    else if (SvTYPE(value) >= SVt_PVMG && SvMAGIC(value) &&
-		     SvTYPE(value) != SVt_PVGV)
-	    {
-		SvFLAGS(value) |= (SvFLAGS(value) &
-				  (SVp_NOK|SVp_POK)) >> PRIVSHIFT;
-		SvMAGICAL_off(value);
-		/* XXX this is a leak when we get here because the
-		 * mg_get() in save_scalar_at() croaked */
-		SvMAGIC(value) = 0;
-	    }
-	    SvREFCNT_dec(sv);
-	    *(SV**)ptr = value;
-	    PL_localizing = 2;
-	    SvSETMAGIC(value);
-	    PL_localizing = 0;
-	    SvREFCNT_dec(value);
+	    restore_sv(value, (SV**)SSPOPPTR);
 	    break;
 	case SAVEt_AV:				/* array reference */
 	    av = (AV*)SSPOPPTR;
@@ -951,8 +953,9 @@
 		if (sv && sv != &PL_sv_undef) {
 		    if (SvTIED_mg((SV*)av, PERL_MAGIC_tied))
 			(void)SvREFCNT_inc(sv);
+		    restore_sv(value, (SV**)ptr);
 		    SvREFCNT_dec(av);
-		    goto restore_sv;
+		    break;
 		}
 	    }
 	    SvREFCNT_dec(av);
@@ -969,9 +972,10 @@
 		    ptr = &HeVAL((HE*)ptr);
 		    if (SvTIED_mg((SV*)hv, PERL_MAGIC_tied))
 			(void)SvREFCNT_inc(*(SV**)ptr);
-		    SvREFCNT_dec(hv);
 		    SvREFCNT_dec(sv);
-		    goto restore_sv;
+		    restore_sv(value, (SV**)ptr);
+		    SvREFCNT_dec(hv);
+		    break;
 		}
 	    }
 	    SvREFCNT_dec(hv);
--- t/op/local.t.old	Mon May  6 17:36:23 2002
+++ t/op/local.t	Thu Jul 18 15:38:00 2002
@@ -1,6 +1,6 @@
 #!./perl
 
-print "1..75\n";
+print "1..76\n";
 
 sub foo {
     local($a, $b) = @_;
@@ -257,3 +257,10 @@
 print "not " if exists $h{'z'}; print "ok 73\n";
 print "not " if exists $ENV{_A_}; print "ok 74\n";
 print "not " if exists $ENV{_B_}; print "ok 75\n";
+
+{
+    # [perl #1176] Attempt to free unreferenced scalar (in 49 lines)
+    sub localiser { local $_[0] }
+    localiser();
+    print "ok 76\n";
+}

@p5pRT
Copy link
Author

p5pRT commented Jul 18, 2002

From @nwc10

On Thu, Jul 18, 2002 at 05​:01​:57PM -0000, Hugo van der Sanden wrote​:

[Too late for 5.8.0, sorry.]

On Thu, Jul 18, 2002 at 06​:06​:25PM +0100, Hugo van der Sanden wrote​:

[Too late for 5.8.0, sorry.]

Would it be possible for RT not to change the message ID on the messages
it spots passing through p5p? I've got a de-dup filter running on message ID,
(I believe many people have) and so I received 2 copies of Hugo's message
into my inbox​: 1 (either direct or via p5p, the second would be de-duped) plus
1 helpfully sent by RT because I was the bug originator, but not with the
same message ID as the other 2.

Or does RT rely on message IDs, and really really need to send out its
messages with IDs it knows about, so not changing them would fundamentally
break it?

Nicholas Clark once wrote​:

That was August 1999, or thereabouts, I think. That bug is the most
substantial legacy of my work at boo.com
(I believe that all the code written by and for us effectively got rm -rf'd
ages ago, although I wouldn't eat my shorts if someone dug up a backup of
the CVS repository.)

:I've got this as short as possible.

<giggle>

Hrumph. It took me ages to get the test case down to 49 lines.

This script​:
perl -we 'sub f { local $_[0] } f()'
coredumps in leave_scope, because (to try and take advantage of a goto)
it frees the av before restoring the sv into it.

Fixing that still leaves the 'attempt to free' problems in the original
code, which can be reduced to​:
crypt% ./perl -we 'sub a { local($_[1],$_[2],$_[3]) } sub b { $x++ && a(0..2) for 1..2 } b() for 1..2'

and now you've got it down to 1 slightly long line :-(
And that's without using Unicode. On the basis that with Unicode I'm expecting
that all programs will be reducable to 1 liners, given the exponential
increase in possible combinations of characters we will have :-)

Name "main​::x" used only once​: possible typo at -e line 1.
Attempt to free unreferenced scalar at -e line 1.
Attempt to free temp prematurely​: SV 0x814b288 at -e line 1.
Attempt to free unreferenced scalar at -e line 1.
crypt%
... which I haven't yet attempted to investigate.

This is already way too scary for me.

I note that this test case still coredumps when run with '-Ds', though
it gets further than without the patch below. I suspect that's
a separate problem, probably due to bitrot in the rarely used -Ds code.

Sounds like we ought to work out a way of running the regression tests with
each -D option in turn.

The patch below breaks out the code it wants to goto, so that we can
conveniently call it before the SvREFCNT_dec(av), and fixes up similar
problems in parallel branches.

Nicholas Clark
--
Even better than the real thing​: http​://nms-cgi.sourceforge.net/

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2002

From @rspier

That particular message was not supposed to be sent out, the first
time, or the second time. It leaked out during the import process.
(And that was before I got tired.)

Moving forward (from yesterday) the only messages that RT should send
directly to p5p are the new ticket notifications.

-R

Nicholas Clark writes​:

On Thu, Jul 18, 2002 at 05​:01​:57PM -0000, Hugo van der Sanden wrote​:

[Too late for 5.8.0, sorry.]

On Thu, Jul 18, 2002 at 06​:06​:25PM +0100, Hugo van der Sanden wrote​:

[Too late for 5.8.0, sorry.]

Would it be possible for RT not to change the message ID on the messages
it spots passing through p5p? I've got a de-dup filter running on message ID,
(I believe many people have) and so I received 2 copies of Hugo's message
into my inbox​: 1 (either direct or via p5p, the second would be de-duped) plus
1 helpfully sent by RT because I was the bug originator, but not with the
same message ID as the other 2.

Or does RT rely on message IDs, and really really need to send out its
messages with IDs it knows about, so not changing them would fundamentally
break it?

Nicholas Clark once wrote​:

That was August 1999, or thereabouts, I think. That bug is the most
substantial legacy of my work at boo.com
(I believe that all the code written by and for us effectively got rm -rf'd
ages ago, although I wouldn't eat my shorts if someone dug up a backup of
the CVS repository.)

@p5pRT
Copy link
Author

p5pRT commented Apr 22, 2003

From @iabyn

I'm closing this old bug report because I think its been fixed by patch
#19064

@p5pRT
Copy link
Author

p5pRT commented Apr 22, 2003

@iabyn - Status changed from 'open' to 'resolved'

@p5pRT p5pRT closed this as completed Apr 22, 2003
@p5pRT
Copy link
Author

p5pRT commented Nov 28, 2003

From The RT System itself

this still causes attempt to free unreferenced scalar in bleadperl DEVEL7093
on linux

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant