From jgmyers@proofpoint.com

Created by jgmyers@proofpoint.com

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.
 

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

From @nwc10

Thanks for reporting this bug.

On Thu, Mar 16, 2006 at 02​:53​:38PM -0800, John Gardiner Myers wrote​:

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

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

From jgmyers@proofpoint.com

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?

From @gisle

Nicholas Clark <nick@​ccl4.org> writes​:

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

From jgmyers@proofpoint.com

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.

From @nwc10

On Thu, Mar 16, 2006 at 03​:31​:26PM -0800, John Myers wrote​:

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

From @steve-m-hay

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.

From guest@guest.guest.xxxxxxxx

[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)

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