bad assumption in Perl_reg_temp_copy #12110

p5pRT · 2012-05-16T17:01:50Z

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

Searchable as RT112962$

p5pRT · 2012-05-16T17:01:50Z

From @fperrad

Created by perrad@cpan.org

When I update my module re::engine::Lua for Perl 5.12 & 5.14,
qr// segfaults in the function Perl_reg_temp_copy (regcomp.c).
Because this function assumes that the field offs of REGEXP
is always set by the method comp of the regex_engine.
But in re::engine::Lua, offs is set by the method exec.

re::engine::Lua 0.08 includes a workaround,
but I propose a patch.

Perl Info


Flags:
    category=core
    severity=low

Site configuration information for perl 5.14.2:

Configured by Debian Project at Fri Mar 23 17:28:34 UTC 2012.

Summary of my perl5 (revision 5 version 14 subversion 2) configuration:

  Platform:
    osname=linux, osvers=2.6.24-31-server,
archname=i686-linux-gnu-thread-multi-64int
    uname='linux palmer 2.6.24-31-server #1 smp tue feb 14 14:08:16
utc 2012 i686 i686 i386 gnulinux '
    config_args='-Dusethreads -Duselargefiles -Dccflags=-DDEBIAN
-Dcccdlflags=-fPIC -Darchname=i686-linux-gnu -Dprefix=/usr
-Dprivlib=/usr/share/perl/5.14 -Darchlib=/usr/lib/perl/5.14
-Dvendorprefix=/usr -Dvendorlib=/usr/share/perl5
-Dvendorarch=/usr/lib/perl5 -Dsiteprefix=/usr/local
-Dsitelib=/usr/local/share/perl/5.14.2
-Dsitearch=/usr/local/lib/perl/5.14.2 -Dman1dir=/usr/share/man/man1
-Dman3dir=/usr/share/man/man3 -Dsiteman1dir=/usr/local/man/man1
-Dsiteman3dir=/usr/local/man/man3 -Duse64bitint -Dman1ext=1
-Dman3ext=3perl -Dpager=/usr/bin/sensible-pager -Uafs -Ud_csh
-Ud_ualarm -Uusesfio -Uusenm -Ui_libutil -DDEBUGGING=-g -Doptimize=-O2
-Duseshrplib -Dlibperl=libperl.so.5.14.2 -des'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=undef, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN
-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include
-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2 -g',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fno-strict-aliasing
-pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.6.3', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long long', ivsize=8, nvtype='double', nvsize=8,
Off_t='off_t', lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib /lib/i386-linux-gnu /lib/../lib
/usr/lib/i386-linux-gnu /usr/lib/../lib /lib /usr/lib
    libs=-lgdbm -lgdbm_compat -ldb -ldl -lm -lpthread -lc -lcrypt
    perllibs=-ldl -lm -lpthread -lc -lcrypt
    libc=, so=so, useshrplib=true, libperl=libperl.so.5.14.2
    gnulibc_version='2.15'
  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'

Locally applied patches:
From 84d2d39de77ef762886a6215a787410a5a9563d1 Mon Sep 17 00:00:00 2001
From: Francois Perrad <francois.perrad@gadz.org>
Date: Wed, 16 May 2012 17:55:22 +0200
Subject: [PATCH] copy .offs only if not null.


 regcomp.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/regcomp.c b/regcomp.c
index 143f349..c3a258a 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -12729,7 +12729,6 @@ Perl_reg_temp_copy (pTHX_ REGEXP *ret_x, REGEXP *rx)
 {
     struct regexp *ret;
     struct regexp *const r = (struct regexp *)SvANY(rx);
-    register const I32 npar = r->nparens+1;

     PERL_ARGS_ASSERT_REG_TEMP_COPY;

@@ -12749,8 +12748,11 @@ Perl_reg_temp_copy (pTHX_ REGEXP *ret_x, REGEXP *rx)
     SvLEN_set(ret_x, 0);
     SvSTASH_set(ret_x, NULL);
     SvMAGIC_set(ret_x, NULL);
-    Newx(ret->offs, npar, regexp_paren_pair);
-    Copy(r->offs, ret->offs, npar, regexp_paren_pair);
+    if (r->offs) {
+        const I32 npar = r->nparens+1;
+        Newx(ret->offs, npar, regexp_paren_pair);
+        Copy(r->offs, ret->offs, npar, regexp_paren_pair);
+    }
     if (r->substrs) {
         Newx(ret->substrs, 1, struct reg_substr_data);
 	StructCopy(r->substrs, ret->substrs, struct reg_substr_data);
--
1.7.9.5




@INC for perl 5.14.2:
    /etc/perl
    /usr/local/lib/perl/5.14.2
    /usr/local/share/perl/5.14.2
    /usr/lib/perl5
    /usr/share/perl5
    /usr/lib/perl/5.14
    /usr/share/perl/5.14
    /usr/local/lib/site_perl
    .


Environment for perl 5.14.2:
    HOME=/home/user
    LANG=en_US.UTF-8
    LANGUAGE=en_US:en
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/usr/lib/lightdm/lightdm:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games
    PERL_BADLANG (unset)
    SHELL=/bin/bash

p5pRT · 2012-05-24T03:10:05Z

From @jkeenan

On Wed May 16 10:01:50 2012, fperrad wrote:

This is a bug report for perl from perrad@cpan.org,
generated with the help of perlbug 1.39 running under perl 5.14.2.

-----------------------------------------------------------------
[Please describe your issue here]
When I update my module re::engine::Lua for Perl 5.12 & 5.14,
qr// segfaults in the function Perl_reg_temp_copy (regcomp.c).
Because this function assumes that the field offs of REGEXP
is always set by the method comp of the regex_engine.
But in re::engine::Lua, offs is set by the method exec.

re::engine::Lua 0.08 includes a workaround,
but I propose a patch.

[snip]

Locally applied patches:
From 84d2d39de77ef762886a6215a787410a5a9563d1 Mon Sep 17 00:00:00 2001
From: Francois Perrad <francois.perrad@gadz.org>
Date: Wed, 16 May 2012 17:55:22 +0200
Subject: [PATCH] copy .offs only if not null.

---
regcomp.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/regcomp.c b/regcomp.c
index 143f349..c3a258a 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -12729,7 +12729,6 @@ Perl_reg_temp_copy (pTHX_ REGEXP *ret_x,
REGEXP *rx)
{
struct regexp *ret;
struct regexp *const r = (struct regexp *)SvANY(rx);
- register const I32 npar = r->nparens+1;
 PERL\_ARGS\_ASSERT\_REG\_TEMP\_COPY;
@@ -12749,8 +12748,11 @@ Perl_reg_temp_copy (pTHX_ REGEXP *ret_x,
REGEXP *rx)
SvLEN_set(ret_x, 0);
SvSTASH_set(ret_x, NULL);
SvMAGIC_set(ret_x, NULL);
- Newx(ret->offs, npar, regexp_paren_pair);
- Copy(r->offs, ret->offs, npar, regexp_paren_pair);
+ if (r->offs) {
+ const I32 npar = r->nparens+1;
+ Newx(ret->offs, npar, regexp_paren_pair);
+ Copy(r->offs, ret->offs, npar, regexp_paren_pair);
+ }
if (r->substrs) {
Newx(ret->substrs, 1, struct reg_substr_data);
StructCopy(r->substrs, ret->substrs, struct reg_substr_data);
--
1.7.9.5

I tested this patch on blead on Darwin/PPC and it passed all current
tests. It should be evaluated by others for how well it deals with the
problem François described.

Thank you very much.
Jim Keenan

p5pRT · 2012-05-24T03:10:05Z

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

p5pRT · 2012-06-20T01:08:58Z

From @cpansprout

On Wed May 23 20:10:05 2012, jkeenan wrote:

I tested this patch on blead on Darwin/PPC and it passed all current
tests. It should be evaluated by others for how well it deals with the
problem Fran�ois described.

Could someone who understands the regexp plugin interface please comment
on this?

--

Father Chrysostomos

p5pRT · 2012-06-20T07:16:34Z

From @hvds

"Father Chrysostomos via RT" <perlbug-followup@perl.org> wrote:
:On Wed May 23 20:10:05 2012, jkeenan wrote:
:> I tested this patch on blead on Darwin/PPC and it passed all current
:> tests. It should be evaluated by others for how well it deals with the
:> problem Fran�ois described.
:
:Could someone who understands the regexp plugin interface please comment
:on this?

I'm not such a person. The change does not look unreasonable to me, on a
5-minute look; but I'd need to dig further to be sure it was safe simply
to fail to set ret->offs (rather than, say, setting it to NULL). I'd also
be rather happier if the patch came with a test, though by the sounds of
it that's unlikely to be possible in pure perl code.

Hugo

p5pRT · 2012-06-20T10:35:29Z

From @iabyn

On Wed, Jun 20, 2012 at 07:42:24AM +0100, hv@crypt.org wrote:

"Father Chrysostomos via RT" <perlbug-followup@perl.org> wrote:
:On Wed May 23 20:10:05 2012, jkeenan wrote:
:> I tested this patch on blead on Darwin/PPC and it passed all current
:> tests. It should be evaluated by others for how well it deals with the
:> problem Fran�ois described.
:
:Could someone who understands the regexp plugin interface please comment
:on this?

It looks good to me.

I'm not such a person. The change does not look unreasonable to me, on a
5-minute look; but I'd need to dig further to be sure it was safe simply
to fail to set ret->offs (rather than, say, setting it to NULL). I'd also
be rather happier if the patch came with a test, though by the sounds of
it that's unlikely to be possible in pure perl code.

The new regex structure is initially nulled, so it should be safe.
The regex API does't appear to be tested - certainly there's nothing
in ext/XS-APItest/.

--
More than any other time in history, mankind faces a crossroads. One path
leads to despair and utter hopelessness. The other, to total extinction.
Let us pray we have the wisdom to choose correctly.
-- Woody Allen

p5pRT · 2012-06-23T13:38:59Z

From @cpansprout

On Wed May 16 10:01:50 2012, fperrad wrote:

From 84d2d39de77ef762886a6215a787410a5a9563d1 Mon Sep 17 00:00:00 2001
From: Francois Perrad <francois.perrad@gadz.org>
Date: Wed, 16 May 2012 17:55:22 +0200
Subject: [PATCH] copy .offs only if not null.

---
regcomp.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/regcomp.c b/regcomp.c
index 143f349..c3a258a 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -12729,7 +12729,6 @@ Perl_reg_temp_copy (pTHX_ REGEXP *ret_x,
REGEXP *rx)
{
struct regexp *ret;
struct regexp *const r = (struct regexp *)SvANY(rx);
- register const I32 npar = r->nparens+1;
 PERL\_ARGS\_ASSERT\_REG\_TEMP\_COPY;
@@ -12749,8 +12748,11 @@ Perl_reg_temp_copy (pTHX_ REGEXP *ret_x,
REGEXP *rx)
SvLEN_set(ret_x, 0);
SvSTASH_set(ret_x, NULL);
SvMAGIC_set(ret_x, NULL);
- Newx(ret->offs, npar, regexp_paren_pair);
- Copy(r->offs, ret->offs, npar, regexp_paren_pair);
+ if (r->offs) {
+ const I32 npar = r->nparens+1;
+ Newx(ret->offs, npar, regexp_paren_pair);
+ Copy(r->offs, ret->offs, npar, regexp_paren_pair);
+ }
if (r->substrs) {
Newx(ret->substrs, 1, struct reg_substr_data);
StructCopy(r->substrs, ret->substrs, struct reg_substr_data);
--
1.7.9.5

Thank you. Applied as 7746563.
--

Father Chrysostomos

p5pRT · 2012-06-23T13:39:00Z

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

p5pRT closed this as completed Jun 23, 2012

p5pRT added Severity Low distro-Linux type-core labels Oct 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bad assumption in Perl_reg_temp_copy #12110

bad assumption in Perl_reg_temp_copy #12110

p5pRT commented May 16, 2012

p5pRT commented May 16, 2012

p5pRT commented May 24, 2012

p5pRT commented May 24, 2012

p5pRT commented Jun 20, 2012

p5pRT commented Jun 20, 2012

p5pRT commented Jun 20, 2012

p5pRT commented Jun 23, 2012

p5pRT commented Jun 23, 2012

bad assumption in Perl_reg_temp_copy #12110

bad assumption in Perl_reg_temp_copy #12110

Comments

p5pRT commented May 16, 2012

p5pRT commented May 16, 2012

From @fperrad

Created by perrad@cpan.org

p5pRT commented May 24, 2012

From @jkeenan

p5pRT commented May 24, 2012

p5pRT commented Jun 20, 2012

From @cpansprout

p5pRT commented Jun 20, 2012

From @hvds

p5pRT commented Jun 20, 2012

From @iabyn

p5pRT commented Jun 23, 2012

From @cpansprout

p5pRT commented Jun 23, 2012