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

Multiple segfaults in Storable. #15714

Closed
p5pRT opened this issue Nov 14, 2016 · 14 comments
Closed

Multiple segfaults in Storable. #15714

p5pRT opened this issue Nov 14, 2016 · 14 comments

Comments

@p5pRT
Copy link

p5pRT commented Nov 14, 2016

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

Searchable as RT130098$

@p5pRT
Copy link
Author

p5pRT commented Nov 14, 2016

From @lightsey

This is a bug report for perl from john@​nixnuts.net,
generated with the help of perlbug 1.40 running under perl 5.25.7.


AFL pointed out multiple null pointer dereference bugs in Storable.

retrieve_code() will dereference a null "text" variable,
and retrieve_other() will dereference a null "cxt" variable.

I'll attach a patch that fixes the issues AFL identified.



Flags​:
  category=library
  severity=low
  module=Storable


Site configuration information for perl 5.25.7​:

Configured by jd at Wed Nov 9 09​:22​:44 CST 2016.

Summary of my perl5 (revision 5 version 25 subversion 7) configuration​:
  Snapshot of​: cc2b46b
  Platform​:
  osname=linux
  osvers=4.8.0-1-amd64
  archname=x86_64-linux
  uname='linux slug 4.8.0-1-amd64 #1 smp debian 4.8.5-1 (2016-10-28) x86_64 gnulinux '
  config_args='-de -Dprefix=/home/jd/perl5/perlbrew/perls/perl-blead -Dcc=/usr/bin/afl-gcc -DDEBUGGING -Dusedevel -Aeval​:scriptdir=/home/jd/perl5/perlbrew/perls/perl-blead/bin'
  hint=recommended
  useposix=true
  d_sigaction=define
  useithreads=undef
  usemultiplicity=undef
  use64bitint=define
  use64bitall=define
  uselongdouble=undef
  usemymalloc=n
  bincompat5005=undef
  Compiler​:
  cc='/usr/bin/afl-gcc'
  ccflags ='-fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'
  optimize='-O2 -g'
  cppflags='-fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
  ccversion=''
  gccversion='6.2.0 20161103'
  gccosandvers=''
  intsize=4
  longsize=8
  ptrsize=8
  doublesize=8
  byteorder=12345678
  doublekind=3
  d_longlong=define
  longlongsize=8
  d_longdbl=define
  longdblsize=16
  longdblkind=3
  ivtype='long'
  ivsize=8
  nvtype='double'
  nvsize=8
  Off_t='off_t'
  lseeksize=8
  alignbytes=8
  prototype=define
  Linker and Libraries​:
  ld='/usr/bin/afl-gcc'
  ldflags =' -fstack-protector-strong -L/usr/local/lib'
  libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/6/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib
  libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  libc=libc-2.24.so
  so=so
  useshrplib=false
  libperl=libperl.a
  gnulibc_version='2.24'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs
  dlext=so
  d_dlsymun=undef
  ccdlflags='-Wl,-E'
  cccdlflags='-fPIC'
  lddlflags='-shared -O2 -g -L/usr/local/lib -fstack-protector-strong'

Locally applied patches​:
  Devel​::PatchPerl 1.44


@​INC for perl 5.25.7​:
  /home/jd/perl5/perlbrew/perls/perl-blead/lib/site_perl/5.25.7/x86_64-linux
  /home/jd/perl5/perlbrew/perls/perl-blead/lib/site_perl/5.25.7
  /home/jd/perl5/perlbrew/perls/perl-blead/lib/5.25.7/x86_64-linux
  /home/jd/perl5/perlbrew/perls/perl-blead/lib/5.25.7


Environment for perl 5.25.7​:
  HOME=/home/jd
  LANG=en_US.UTF-8
  LANGUAGE (unset)
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)
  PATH=/home/jd/perl5/perlbrew/bin​:/home/jd/perl5/perlbrew/perls/perl-blead/bin​:/home/jd/.gems/bin​:/home/jd/bin​:/home/jd/bin​:/usr/local/bin​:/usr/bin​:/bin​:/usr/local/games​:/usr/games
  PERLBREW_BASHRC_VERSION=0.76
  PERLBREW_HOME=/home/jd/.perlbrew
  PERLBREW_MANPATH=/home/jd/perl5/perlbrew/perls/perl-blead/man
  PERLBREW_PATH=/home/jd/perl5/perlbrew/bin​:/home/jd/perl5/perlbrew/perls/perl-blead/bin
  PERLBREW_PERL=perl-blead
  PERLBREW_ROOT=/home/jd/perl5/perlbrew
  PERLBREW_VERSION=0.76
  PERL_BADLANG (unset)
  SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Nov 14, 2016

From @lightsey

Patch for this issue.

@p5pRT
Copy link
Author

p5pRT commented Nov 14, 2016

From @lightsey

0001-Fix-Storable-segfaults.patch
From fecd3be8dbdb747b9cbf4cbb9299ce40faabc8e6 Mon Sep 17 00:00:00 2001
From: John Lightsey <lightsey@debian.org>
Date: Mon, 14 Nov 2016 11:56:15 +0100
Subject: [PATCH] Fix Storable segfaults.

Fix a null pointed dereference segfault in storable when the
retrieve_code logic was unable to read the string that contained
the code.

Also fix several locations where retrieve_other was called with a
null context pointer. This also resulted in a null pointer
dereference.
---
 dist/Storable/Storable.xs | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs
index 053951c..caa489c 100644
--- a/dist/Storable/Storable.xs
+++ b/dist/Storable/Storable.xs
@@ -5647,6 +5647,10 @@ static SV *retrieve_code(pTHX_ stcxt_t *cxt, const char *cname)
 		CROAK(("Unexpected type %d in retrieve_code\n", type));
 	}
 
+	if (!text) {
+		CROAK(("Unable to retrieve code\n"));
+	}
+
 	/*
 	 * prepend "sub " to the source
 	 */
@@ -5767,7 +5771,7 @@ static SV *old_retrieve_array(pTHX_ stcxt_t *cxt, const char *cname)
 			continue;			/* av_extend() already filled us with undef */
 		}
 		if (c != SX_ITEM)
-			(void) retrieve_other(aTHX_ (stcxt_t *) 0, 0);	/* Will croak out */
+			(void) retrieve_other(aTHX_ cxt, 0);	/* Will croak out */
 		TRACEME(("(#%d) item", i));
 		sv = retrieve(aTHX_ cxt, 0);						/* Retrieve item */
 		if (!sv)
@@ -5844,7 +5848,7 @@ static SV *old_retrieve_hash(pTHX_ stcxt_t *cxt, const char *cname)
 			if (!sv)
 				return (SV *) 0;
 		} else
-			(void) retrieve_other(aTHX_ (stcxt_t *) 0, 0);	/* Will croak out */
+			(void) retrieve_other(aTHX_ cxt, 0);	/* Will croak out */
 
 		/*
 		 * Get key.
@@ -5855,7 +5859,7 @@ static SV *old_retrieve_hash(pTHX_ stcxt_t *cxt, const char *cname)
 
 		GETMARK(c);
 		if (c != SX_KEY)
-			(void) retrieve_other(aTHX_ (stcxt_t *) 0, 0);	/* Will croak out */
+			(void) retrieve_other(aTHX_ cxt, 0);	/* Will croak out */
 		RLEN(size);						/* Get key size */
 		KBUFCHK((STRLEN)size);					/* Grow hash key read pool if needed */
 		if (size)
-- 
2.10.2

@p5pRT
Copy link
Author

p5pRT commented Nov 14, 2016

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Dec 1, 2016

From @lightsey

Updated patch to correct several null pointer deference bugs in Storable is
attached.

@p5pRT
Copy link
Author

p5pRT commented Dec 1, 2016

From @lightsey

0001-Fix-Storable-segfaults.patch
From d1964dfd8a2d38a29c5dbb94c0cf1ac46d022fff Mon Sep 17 00:00:00 2001
From: John Lightsey <lightsey@debian.org>
Date: Mon, 14 Nov 2016 11:56:15 +0100
Subject: [PATCH] Fix Storable segfaults.

Fix a null pointer dereference segfault in storable when the
retrieve_code logic was unable to read the string that contained
the code.

Also fix several locations where retrieve_other was called with a
null context pointer. This also resulted in a null pointer
dereference.
---
 Porting/checkAUTHORS.pl   |  1 +
 dist/Storable/Storable.pm |  2 +-
 dist/Storable/Storable.xs | 10 +++++++---
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/Porting/checkAUTHORS.pl b/Porting/checkAUTHORS.pl
index b869c11..fcddf51 100755
--- a/Porting/checkAUTHORS.pl
+++ b/Porting/checkAUTHORS.pl
@@ -660,6 +660,7 @@ jasons\100cs.unm.edu                    jasons\100sandy-home.arc.unm.edu
 jbuehler\100hekimian.com                jhpb\100hekimian.com
 jcromie\100100divsol.com                jcromie\100cpan.org
 +                                       jim.cromie\100gmail.com
+jd\100cpanel.net                        lightsey\100debian.org
 jdhedden\100cpan.org                    jerry\100hedden.us
 +                                       jdhedden\1001979.usna.com
 +                                       jdhedden\100gmail.com
diff --git a/dist/Storable/Storable.pm b/dist/Storable/Storable.pm
index 7101641..18aa1f0 100644
--- a/dist/Storable/Storable.pm
+++ b/dist/Storable/Storable.pm
@@ -22,7 +22,7 @@ package Storable; @ISA = qw(Exporter);
 
 use vars qw($canonical $forgive_me $VERSION);
 
-$VERSION = '2.59';
+$VERSION = '2.60';
 
 BEGIN {
     if (eval {
diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs
index 3788f57..a72d84c 100644
--- a/dist/Storable/Storable.xs
+++ b/dist/Storable/Storable.xs
@@ -5660,6 +5660,10 @@ static SV *retrieve_code(pTHX_ stcxt_t *cxt, const char *cname)
 		CROAK(("Unexpected type %d in retrieve_code\n", type));
 	}
 
+	if (!text) {
+		CROAK(("Unable to retrieve code\n"));
+	}
+
 	/*
 	 * prepend "sub " to the source
 	 */
@@ -5780,7 +5784,7 @@ static SV *old_retrieve_array(pTHX_ stcxt_t *cxt, const char *cname)
 			continue;			/* av_extend() already filled us with undef */
 		}
 		if (c != SX_ITEM)
-			(void) retrieve_other(aTHX_ (stcxt_t *) 0, 0);	/* Will croak out */
+			(void) retrieve_other(aTHX_ cxt, 0);	/* Will croak out */
 		TRACEME(("(#%d) item", i));
 		sv = retrieve(aTHX_ cxt, 0);						/* Retrieve item */
 		if (!sv)
@@ -5857,7 +5861,7 @@ static SV *old_retrieve_hash(pTHX_ stcxt_t *cxt, const char *cname)
 			if (!sv)
 				return (SV *) 0;
 		} else
-			(void) retrieve_other(aTHX_ (stcxt_t *) 0, 0);	/* Will croak out */
+			(void) retrieve_other(aTHX_ cxt, 0);	/* Will croak out */
 
 		/*
 		 * Get key.
@@ -5868,7 +5872,7 @@ static SV *old_retrieve_hash(pTHX_ stcxt_t *cxt, const char *cname)
 
 		GETMARK(c);
 		if (c != SX_KEY)
-			(void) retrieve_other(aTHX_ (stcxt_t *) 0, 0);	/* Will croak out */
+			(void) retrieve_other(aTHX_ cxt, 0);	/* Will croak out */
 		RLEN(size);						/* Get key size */
 		KBUFCHK((STRLEN)size);					/* Grow hash key read pool if needed */
 		if (size)
-- 
2.10.2

@p5pRT
Copy link
Author

p5pRT commented Dec 1, 2016

From @jkeenan

On Thu, 01 Dec 2016 02​:36​:15 GMT, john@​nixnuts.net wrote​:

Updated patch to correct several null pointer deference bugs in Storable is
attached.

Can you provide a bit more specific evidence of the problem?

Are there any tests we could write for this that would expose regressions?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Dec 2, 2016

From @lightsey

On Wed, 2016-11-30 at 19​:19 -0800, James E Keenan via RT wrote​:

On Thu, 01 Dec 2016 02​:36​:15 GMT, john@​nixnuts.net wrote​:

Updated patch to correct several null pointer deference bugs in Storable is
attached.

Can you provide a bit more specific evidence of the problem?

Are there any tests we could write for this that would expose regressions?

Thank you very much.

I've bundled up three of the crashing Storable files into the attached test
script. These segfault reliably on my x86_64 system in three of the patched
locations.

$ gdb -ex run -ex bt -batch -args perl ./null_crashes.pl old_retrieve_array
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Loading old_retrieve_array...

Program received signal SIGSEGV, Segmentation fault.
retrieve_other (cxt=cxt@​entry=0x0, cname=0x0) at Storable.xs​:3984
3984                    cxt->ver_major != STORABLE_BIN_MAJOR &&
#0  retrieve_other (cxt=cxt@​entry=0x0, cname=0x0) at Storable.xs​:3984
#1  0x00007ffff6872967 in old_retrieve_array (cxt=0x555556355020,
cname=<optimized out>) at Storable.xs​:5783
...

@p5pRT
Copy link
Author

p5pRT commented Dec 2, 2016

From @lightsey

null_crashes.pl

@p5pRT
Copy link
Author

p5pRT commented Dec 25, 2016

From @jkeenan

On Fri, 02 Dec 2016 02​:32​:34 GMT, john@​nixnuts.net wrote​:

On Wed, 2016-11-30 at 19​:19 -0800, James E Keenan via RT wrote​:

On Thu, 01 Dec 2016 02​:36​:15 GMT, john@​nixnuts.net wrote​:

Updated patch to correct several null pointer deference bugs in
Storable is
attached.

Can you provide a bit more specific evidence of the problem?

Are there any tests we could write for this that would expose
regressions?

Thank you very much.

I've bundled up three of the crashing Storable files into the attached
test
script. These segfault reliably on my x86_64 system in three of the
patched
locations.

$ gdb -ex run -ex bt -batch -args perl ./null_crashes.pl
old_retrieve_array
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-
gnu/libthread_db.so.1".
Loading old_retrieve_array...

Program received signal SIGSEGV, Segmentation fault.
retrieve_other (cxt=cxt@​entry=0x0, cname=0x0) at Storable.xs​:3984
3984                    cxt->ver_major != STORABLE_BIN_MAJOR &&
#0  retrieve_other (cxt=cxt@​entry=0x0, cname=0x0) at Storable.xs​:3984
#1  0x00007ffff6872967 in old_retrieve_array (cxt=0x555556355020,
cname=<optimized out>) at Storable.xs​:5783
...

John,

In the smoke-me/jkeenan/130098-storable branch I applied your patch for Storable.xs. Storable​::$VERSION had already been incremented, so I manually re-incremented. I worked your test program into 3 regression tests in t/store.t -- though you might want to suggest better descriptions than mine.

P5P​: Could this be reviewed by someone more familiar with Storable than I? (Otherwise, I will push to blead within 7 days.)

Thank you very much.
--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Jan 1, 2017

From @jkeenan

On Sun, 25 Dec 2016 02​:46​:38 GMT, jkeenan wrote​:

On Fri, 02 Dec 2016 02​:32​:34 GMT, john@​nixnuts.net wrote​:

On Wed, 2016-11-30 at 19​:19 -0800, James E Keenan via RT wrote​:

On Thu, 01 Dec 2016 02​:36​:15 GMT, john@​nixnuts.net wrote​:

Updated patch to correct several null pointer deference bugs in
Storable is
attached.

Can you provide a bit more specific evidence of the problem?

Are there any tests we could write for this that would expose
regressions?

Thank you very much.

I've bundled up three of the crashing Storable files into the
attached
test
script. These segfault reliably on my x86_64 system in three of the
patched
locations.

$ gdb -ex run -ex bt -batch -args perl ./null_crashes.pl
old_retrieve_array
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-
gnu/libthread_db.so.1".
Loading old_retrieve_array...

Program received signal SIGSEGV, Segmentation fault.
retrieve_other (cxt=cxt@​entry=0x0, cname=0x0) at Storable.xs​:3984
3984                    cxt->ver_major != STORABLE_BIN_MAJOR &&
#0  retrieve_other (cxt=cxt@​entry=0x0, cname=0x0) at Storable.xs​:3984
#1  0x00007ffff6872967 in old_retrieve_array (cxt=0x555556355020,
cname=<optimized out>) at Storable.xs​:5783
...

John,

In the smoke-me/jkeenan/130098-storable branch I applied your patch
for Storable.xs. Storable​::$VERSION had already been incremented, so
I manually re-incremented. I worked your test program into 3
regression tests in t/store.t -- though you might want to suggest
better descriptions than mine.

P5P​: Could this be reviewed by someone more familiar with Storable
than I? (Otherwise, I will push to blead within 7 days.)

Thank you very much.

Can you confirm that commit adf9095 remedies this situation?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Jan 5, 2017

From @lightsey

On Sun, 2017-01-01 at 07​:19 -0800, James E Keenan via RT wrote​:

John,
 
In the smoke-me/jkeenan/130098-storable branch I applied your patch
for Storable.xs.  Storable​::$VERSION had already been incremented, so
I manually re-incremented.  I worked your test program into 3
regression tests in t/store.t -- though you might want to suggest
better descriptions than mine.
 
P5P​:  Could this be reviewed by someone more familiar with Storable
than I?  (Otherwise, I will push to blead within 7 days.)
 
Thank you very much.

Can you confirm that commit adf9095 remedies
this situation?

The changes look good and the segfaults are no longer reproducible on my test
system.

@p5pRT
Copy link
Author

p5pRT commented Jan 12, 2017

From @jkeenan

Since the other ticket cited in this one has been marked Resolved, I am marking this one Resolved as well.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Jan 12, 2017

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

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

No branches or pull requests

1 participant