Skip Menu |
 
Report information
Id: 38744
Status: rejected
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: jgmyers <jgmyers [at] proofpoint.com>
Cc:
AdminCc:

Operating System: Linux
PatchStatus: (no value)
Severity: High
Type: notabug
Perl Version: 5.8.8
Fixed In: (no value)



Subject: Undetectable, incompatible API changes in POPpx, etc.
Date: Thu, 16 Mar 2006 14:51:40 -0800
To: perlbug [...] perl.org
From: "John Myers" <jgmyers [...] proofpoint.com>
Download (untitled) / with headers
text/plain 4.4k
This is a bug report for perl from jgmyers@proofpoint.com, generated with the help of perlbug 1.35 running under perl v5.8.8. ----------------------------------------------------------------- [Please enter your report here] Perl 5.8.8 contains an incompatible and undetectable change to the public, documented POPpx macro--the macro no longer assigns the length of the popped value to n_a. This ChangeLog entry appears to be the relevant one: [ 25525] By: nicholas on 2005/09/21 09:32:33 Log: Integrate: [ 24748] Convert POPpx POPpconstx and POPpbytex to use nolen macros. Elminate a lot of C<n_a>s Our code was calling POPpx then using the value assigned to n_a to allocate a buffer into which the returned value was copied. After upgrading to Perl 5.8.8, the code still compiled, but due to the POPpx change then passed the now-uninitialized value of n_a to the allocation routine. Fortunately, the value that happened to be in that memory location caused the allocator to throw an exception, but it could just as well have allocated a short buffer. It was highly irresponsible for someone to make an incompatible change to a documented, public API without ensuring that code depending on the old API caused the compile to fail. The macros should have been renamed or they should have been changed to take a different number of arguments. There is no telling what third party code might now have buffer overflow security bugs due to this incompatible change. [Please do not change anything below this line] ----------------------------------------------------------------- --- Flags: category=core severity=high --- Site configuration information for perl v5.8.8: Configured by jgmyers at Thu Mar 16 13:44:33 PST 2006. Summary of my perl5 (revision 5 version 8 subversion 8) configuration: Platform: osname=linux, osvers=2.4.21-37.elsmp, archname=i686-linux-thread-multi uname='linux pong.us.proofpoint.com 2.4.21-37.elsmp #1 smp wed sep 7 13:28:55 edt 2005 i686 i686 i386 gnulinux ' config_args='' hint=recommended, useposix=true, d_sigaction=define usethreads=define use5005threads=undef 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='gcc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS -DDEBUGGING -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -I/usr/include/gdbm', optimize='-g', cppflags='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS -DDEBUGGING -fno-strict-aliasing -pipe -I/usr/local/include -I/usr/include/gdbm' ccversion='', gccversion='3.3.3', gccosandvers='' intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234 d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12 ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8 alignbytes=4, prototype=define Linker and Libraries: ld='gcc', ldflags =' -L/usr/local/lib' libpth=/usr/local/lib /lib /usr/lib libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lpthread -lc perllibs=-lnsl -ldl -lm -lcrypt -lutil -lpthread -lc libc=/lib/libc-2.3.2.so, so=so, useshrplib=false, libperl=libperl.a gnulibc_version='2.3.2' Dynamic Linking: dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E' cccdlflags='-fpic', lddlflags='-shared -L/usr/local/lib' Locally applied patches: --- @INC for perl v5.8.8: /u/jgmyers/perl/lib/5.8.8/i686-linux-thread-multi /u/jgmyers/perl/lib/5.8.8 /u/jgmyers/perl/lib/site_perl/5.8.8/i686-linux-thread-multi /u/jgmyers/perl/lib/site_perl/5.8.8 /u/jgmyers/perl/lib/site_perl/5.8.7/i686-linux-thread-multi /u/jgmyers/perl/lib/site_perl/5.8.7 /u/jgmyers/perl/lib/site_perl/5.8.6/i686-linux-thread-multi /u/jgmyers/perl/lib/site_perl/5.8.6 /u/jgmyers/perl/lib/site_perl/5.8.5/i686-linux-thread-multi /u/jgmyers/perl/lib/site_perl/5.8.5 /u/jgmyers/perl/lib/site_perl/5.8.3/i686-linux-thread-multi /u/jgmyers/perl/lib/site_perl/5.8.3 /u/jgmyers/perl/lib/site_perl . --- Environment for perl v5.8.8: HOME=/u/jgmyers LANG=en_US LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/tools/x/bin:/usr/kerberos/bin:/usr/local/bin:/bin:/usr/bin:/usr/X11R6/bin:/u/jgmyers/bin PERL_BADLANG (unset) SHELL=/bin/bash
Subject: Re: [perl #38744] Undetectable, incompatible API changes in POPpx, etc.
Date: Thu, 16 Mar 2006 23:25:59 +0000
To: perl5-porters [...] perl.org
From: Nicholas Clark <nick [...] ccl4.org>
Download (untitled) / with headers
text/plain 1.2k
Thanks for reporting this bug. On Thu, Mar 16, 2006 at 02:53:38PM -0800, John Gardiner Myers wrote: Show quoted text
> It was highly irresponsible for someone to make an incompatible change > to a documented, public API without ensuring that code depending on the > old API caused the compile to fail. The macros should have been renamed > or they should have been changed to take a different number of > arguments. There is no telling what third party code might now have > buffer overflow security bugs due to this incompatible change.
The old documentation gives a return value of char *, and states Pops a string off the stack. Requires a variable STRLEN n_a in scope. The new documentation gives a return value of char *, and states Pops a string off the stack. No-one reading the public commit list realised that the changed behaviour could represent a subtle mistake. Likewise no-one spotted it as a result of any of the maintenance snapshots in the 3 months since that change was made, nor was it discovered by anyone testing the release candidate. Of the 5000 modules on CPAN, 2 have code that is relying on n_a being set. Those authors will be notified, but there's no way to track down other code affected. Sorry for the trouble this has caused. Nicholas Clark
Subject: Re: [perl #38744] Undetectable, incompatible API changes in POPpx, etc.
Date: Thu, 16 Mar 2006 15:31:26 -0800
To: perlbug-followup [...] perl.org
From: "John Myers" <jgmyers [...] proofpoint.com>
Download (untitled) / with headers
text/plain 146b
Nicholas Clark via RT wrote: Show quoted text
> Of the 5000 modules on CPAN, 2 have code that is relying on n_a being set.
What are the names of those 2 modules?
CC: perl5-porters [...] perl.org
Subject: Re: [perl #38744] Undetectable, incompatible API changes in POPpx, etc.
Date: 17 Mar 2006 07:36:50 -0800
To: Nicholas Clark <nick [...] ccl4.org>
From: Gisle Aas <gisle [...] ActiveState.com>
Download (untitled) / with headers
text/plain 988b
Nicholas Clark <nick@ccl4.org> writes: Show quoted text
> > It was highly irresponsible for someone to make an incompatible change > > to a documented, public API without ensuring that code depending on the > > old API caused the compile to fail. The macros should have been renamed > > or they should have been changed to take a different number of > > arguments. There is no telling what third party code might now have > > buffer overflow security bugs due to this incompatible change.
> > The old documentation gives a return value of char *, and states > > Pops a string off the stack. > Requires a variable STRLEN n_a in scope. > > The new documentation gives a return value of char *, and states > > Pops a string off the stack. > > No-one reading the public commit list realised that the changed behaviour > could represent a subtle mistake.
Anybody stupid enough to depend on an undocumented side effect that modify a variable called "n_a" does not get my sympathy. --Gisle
Subject: Re: [perl #38744] Undetectable, incompatible API changes in POPpx, etc.
Date: Fri, 17 Mar 2006 07:52:24 -0800
To: perlbug-followup [...] perl.org
From: John Gardiner Myers <jgmyers [...] proofpoint.com>
Download (untitled) / with headers
text/plain 179b
Gisle Aas via RT wrote: Show quoted text
> Anybody stupid enough to depend on an undocumented side effect that > modify a variable called "n_a" does not get my sympathy. >
n_a was documented.
CC: perlbug-followup [...] perl.org
Subject: Re: [perl #38744] Undetectable, incompatible API changes in POPpx, etc.
Date: Fri, 17 Mar 2006 21:22:11 +0000
To: John Myers <jgmyers [...] proofpoint.com>
From: Nicholas Clark <nick [...] ccl4.org>
Download (untitled) / with headers
text/plain 346b
On Thu, Mar 16, 2006 at 03:31:26PM -0800, John Myers wrote: Show quoted text
> Nicholas Clark via RT wrote:
> >Of the 5000 modules on CPAN, 2 have code that is relying on n_a being set.
> What are the names of those 2 modules?
According to cpansearch.bulknews.net, Net::ESMTP and MIME::Fast http://cpansearch.bulknews.net/search?q=POPpx&fm=all Nicholas Clark
CC: perlbug-followup [...] perl.org
Subject: Re: [perl #38744] Undetectable, incompatible API changes in POPpx, etc.
Date: Mon, 20 Mar 2006 09:19:45 +0000
To: John Gardiner Myers <jgmyers [...] proofpoint.com>
From: Steve Hay <steve.hay [...] uk.radan.com>
Download (untitled) / with headers
text/plain 1.2k
John Gardiner Myers wrote: Show quoted text
> Gisle Aas via RT wrote:
>> Anybody stupid enough to depend on an undocumented side effect that >> modify a variable called "n_a" does not get my sympathy. >>
> n_a was documented.
I think the point was that the *setting of* n_a was not documented. The documentation merely stated that such a variable had to be in scope, but made no promise about what it might get set to. It was an undocumented side-effect of POPpx that n_a just happened to get set to what you wanted, but relying on this was not wise. ------------------------------------------------ Radan Computational Ltd. The information contained in this message and any files transmitted with it are confidential and intended for the addressee(s) only. If you have received this message in error or there are any problems, please notify the sender immediately. The unauthorized use, disclosure, copying or alteration of this message is strictly forbidden. Note that any views or opinions presented in this email are solely those of the author and do not necessarily represent those of Radan Computational Ltd. The recipient(s) of this message should check it and any attached files for viruses: Radan Computational will accept no liability for any damage caused by any virus transmitted by this email.
RT-Send-CC: polettix [...] cpan.org
Download (untitled) / with headers
text/plain 2.9k
Show quoted text
> [shay - Mon Mar 20 01:22:10 2006]: > > John Gardiner Myers wrote:
> > Gisle Aas via RT wrote:
> >> Anybody stupid enough to depend on an undocumented side effect that > >> modify a variable called "n_a" does not get my sympathy. > >>
> > n_a was documented.
> > I think the point was that the *setting of* n_a was not documented. > The > documentation merely stated that such a variable had to be in scope, > but > made no promise about what it might get set to. It was an > undocumented > side-effect of POPpx that n_a just happened to get set to what you > wanted, but relying on this was not wise. > > > ------------------------------------------------ > Radan Computational > Ltd. > > The information contained in this message and any files > transmitted with it are confidential and intended for the > addressee(s) only. If you have received this message in error or > there are any problems, please notify the sender immediately. The > unauthorized use, disclosure, copying or alteration of this message > is strictly forbidden. Note that any views or opinions presented in > this email are solely those of the author and do not necessarily > represent those of Radan Computational Ltd. The recipient(s) of > this message should check it and any attached files for viruses: > Radan Computational will accept no liability for any damage caused > by any virus transmitted by this email.
This is legalese and unfair. First of all, the docs in perlapi are extremely succint, and a lot of "imagination" has to be used. This is *not* a critic, but don't blame the reader please. Documentation for POPp clearly stated that it was deprecated in favour of POPpx, and it was natural for a stupid like me to conclude that this was to ensure that bytes were not lost in binary data, and that the docs were simply succint. I worked it around using the following setup: STRLEN n_a; char *tret = SvPVx(POPs, n_a); But now that we're under use strict 'legalese'; I'd like to point out the documentation for SvPV/SvPVx: SvPV Returns a pointer to the string in the SV, or a stringified form of the SV if the SV does not contain a string. The SV may cache the stringified version becoming "SvPOK". Handles 'get' magic. See also "SvPVx" for a version which guarantees to evaluate sv only once. char* SvPV(SV* sv, STRLEN len) SvPVx A version of "SvPV" which guarantees to evaluate sv only once. char* SvPVx(SV* sv, STRLEN len) There is *nothing* that states that len will be assigned the length of the string. Is this something we may rely upon, or am I being stupid one more time? And if so, how could I possibly solve the problem of getting the full string from the stack without continuing to behave as a stupid? (for "being stupid", I fear, there's no remedy in this life). Flavio Poletti (polettix [at] cpan.org)


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org