search

Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.91k stars 542 forks source link

bad assumption in Perl_reg_temp_copy #12110

Closed p5pRT closed 12 years ago

p5pRT commented 12 years ago

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

Searchable as RT112962$

p5pRT commented 12 years ago

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 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 commented 12 years ago

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 commented 12 years ago

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

p5pRT commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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 774656327327593. --

Father Chrysostomos

p5pRT commented 12 years ago

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