sv_vcatpvfn() fails on some (very limited) situations #7976

p5pRT · 2005-06-16T13:36:39Z

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

Searchable as RT36310$

p5pRT · 2005-06-16T13:36:39Z

From ikegami@artemis.a01.aist.go.jp

This is a bug report for perl from ikegami@ni.aist.go.jp,
generated with the help of perlbug 1.35 running under perl v5.8.7.

When Perl 5.8.7 is compiled without -DUSE_LONG_DOUBLE, an API call
sv_vcatpvfn(sv, "%.1f", 4, &va_list, NULL, 0, NULL) doesn't work
properly. It is because an argument (type of double) in va_list is
swallowed during the special treatment for "%.<number>[gf]" format.
A quick patch follows:

Inline Patch

--- sv.c.orig   Thu Jun 16 22:26:21 2005
+++ sv.c        Thu Jun 16 22:27:33 2005
@@ -8403,7 +8403,7 @@
        pp = pat + 2;
        while (*pp >= '0' && *pp <= '9')
            digits = 10 * digits + (*pp++ - '0');
-       if (pp - pat == (int)patlen - 1) {
+       if (pp - pat == (int)patlen - 1 && (*pp == 'g' || !digits)) {
            NV nv;

            if (args)
-----------------------------------------------------------------
---

Flags: category=core severity=low

Site configuration information for perl v5.8.7:

Configured by ikegami at Wed Jun 15 17:01:05 JST 2005.

Summary of my perl5 (revision 5 version 8 subversion 7) configuration:
Platform:
osname=linux, osvers=2.4.18-3, archname=i686-linux-thread-multi
uname='linux artemis.a01.aist.go.jp 2.4.18-3 #1 thu apr 18 07:37:53 edt 2002 i686 unknown '
config_args='-ds -e -Dprefix=/usr -Dusethreads'
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='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -I/usr/include/gdbm',
optimize='-O2',
cppflags='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS -fno-strict-aliasing -pipe -I/usr/local/include -I/usr/include/gdbm'
ccversion='', gccversion='2.96 20000731 (Red Hat Linux 7.3 2.96-110)', 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='cc', ldflags =' -L/usr/local/lib'
libpth=/usr/local/lib /lib /usr/lib
libs=-lnsl -lndbm -lgdbm -ldl -lm -lcrypt -lutil -lpthread -lc
perllibs=-lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
libc=/lib/libc-2.2.5.so, so=so, useshrplib=false, libperl=libperl.a
gnulibc_version='2.2.5'
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.7:
/home/ikegami/lib/perl
/usr/lib/perl5/5.8.7/i686-linux-thread-multi
/usr/lib/perl5/5.8.7
/usr/lib/perl5/site_perl/5.8.7/i686-linux-thread-multi
/usr/lib/perl5/site_perl/5.8.7
/usr/lib/perl5/site_perl/5.6.1
/usr/lib/perl5/site_perl/5.6.0
/usr/lib/perl5/site_perl
.

Environment for perl v5.8.7:
HOME=/home/ikegami
LANG=ja_JP.eucJP
LANGUAGE (unset)
LD_LIBRARY_PATH=/opt/intel_fc_80/lib:/opt/intel/compiler70/ia32/lib
LOGDIR (unset)
PATH=/home/ikegami/bin:/home/ikegami/util:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/X11R6/bin:/opt/intel_fc_80/bin:/opt/intel/compiler70/ia32/bin:/usr/java/j2re1.4.1/bin:/usr/java/j2sdk1.4.1/bin:/home/ikegami/ninf/linux/bin:.
PERL5LIB=/home/ikegami/lib/perl
PERL_BADLANG (unset)
SHELL=/bin/zsh

p5pRT · 2005-06-16T14:25:05Z

From @hvds

"ikegami@artemis.a01.aist.go.jp (via RT)" <perlbug-followup@perl.org> wrote:
:When Perl 5.8.7 is compiled without -DUSE_LONG_DOUBLE, an API call
:sv_vcatpvfn(sv, "%.1f", 4, &va_list, NULL, 0, NULL) doesn't work
:properly. It is because an argument (type of double) in va_list is
:swallowed during the special treatment for "%.<number>[gf]" format.
:A quick patch follows:
:
:--- sv.c.orig Thu Jun 16 22:26:21 2005
:+++ sv.c Thu Jun 16 22:27:33 2005
:@@ -8403,7 +8403,7 @@
: pp = pat + 2;
: while (*pp >= '0' && *pp <= '9')
: digits = 10 * digits + (*pp++ - '0');
:- if (pp - pat == (int)patlen - 1) {
:+ if (pp - pat == (int)patlen - 1 && (*pp == 'g' || !digits)) {
: NV nv;
:
: if (args)

There are in fact several ways we can fall through to the standard code
after grabbing the argument, so all of them are buggy in the same way.

I'm not sure how we can fix the other cases though, since we need to get
the NV and try stuff before we know whether we're going to get an ok result.

I guess it might be possible to introduce an additional flag "I've
already grabbed this arg", but that's going to complicate the code
quite a lot. So I guess the question becomes: how useful is this
optimisation anyway? Do we lose that much if we just rip it out?

In the meantime, the patch below might be useful for testing, and
something similar but less drastic is probably needed for production.
(This patch against blead@24073, the line numbers tend to move around
quite a lot.)

Hmm, I wonder how portable it would be to save a va_list* pointer and
then reuse it, to allow us to step back to arguments previously read
from the list. I'm pretty sure you're not allowed to do that, but I
suspect the vast majority of implementations would cope.

Hugo

Inline Patch

--- sv.c.old	Mon Mar 21 10:06:39 2005
+++ sv.c	Thu Jun 16 15:21:44 2005
@@ -9239,6 +9239,7 @@
 		    return;
 		}
 	    }
+	    Perl_croak(aTHX_ "panic: unsafe fallthrough from /%.\d+[fg]/ optimisation");
 	}
     }
 #endif /* !USE_LONG_DOUBLE */

p5pRT · 2005-06-16T14:25:06Z

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

p5pRT · 2005-06-16T14:36:23Z

From @rgs

hv@crypt.org wrote:

In the meantime, the patch below might be useful for testing, and
something similar but less drastic is probably needed for production.

A warning ?
A mandatory warning ?

Hmm, I wonder how portable it would be to save a va_list* pointer and
then reuse it, to allow us to step back to arguments previously read
from the list. I'm pretty sure you're not allowed to do that, but I
suspect the vast majority of implementations would cope.

Scary.

--- sv.c.old Mon Mar 21 10:06:39 2005
+++ sv.c Thu Jun 16 15:21:44 2005
@@ -9239,6 +9239,7 @@
return;
}
}
+ Perl_croak(aTHX_ "panic: unsafe fallthrough from /%.\d+[fg]/ optimisation");
}
}
#endif /* !USE_LONG_DOUBLE */

p5pRT · 2005-06-16T15:17:59Z

From @hvds

Rafael Garcia-Suarez <rgarciasuarez@mandriva.com> wrote:
:hv@crypt.org wrote:
:>
:> In the meantime, the patch below might be useful for testing, and
:> something similar but less drastic is probably needed for production.
:
:A warning ?
:A mandatory warning ?

I guess a mandatory warning, and return an empty string - falling through
risks a core I think. Or, given that we can't produce the right answer
from this point, maybe croak is appropriate as is.

Hugo

p5pRT · 2005-06-17T08:58:54Z

From t-ikegami@aist.go.jp

From: "Hugo van der Sanden via RT" <perlbug-followup@perl.org>
Date: 16 Jun 2005 14:25:06 -0000

There are in fact several ways we can fall through to the standard code
after grabbing the argument, so all of them are buggy in the same way.

Oops. I've overlooked the case 'g'. For the time being, I'll skip
the block if args != null, removing va_arg() at all.

thanks,
IKEGAMI, Tsutomu
AIST
t-ikegami@aist.go.jp

p5pRT · 2005-06-17T12:20:11Z

From @hvds

Tsutomu IKEGAMI <t-ikegami@aist.go.jp> wrote:
:From: "Hugo van der Sanden via RT" <perlbug-followup@perl.org>
:Date: 16 Jun 2005 14:25:06 -0000
:
:> There are in fact several ways we can fall through to the standard code
:> after grabbing the argument, so all of them are buggy in the same way.
:
:Oops. I've overlooked the case 'g'. For the time being, I'll skip
:the block if args != null, removing va_arg() at all.

Good, that's a much better solution than anything I suggested.

Hugo

p5pRT · 2005-06-20T21:41:12Z

From @rgarcia

On 6/17/05, hv wrote:

:Oops. I've overlooked the case 'g'. For the time being, I'll skip
:the block if args != null, removing va_arg() at all.

Good, that's a much better solution than anything I suggested.

I'm not 100% sure what you meant, is the following correct ? :

==== //depot/perl/sv.c#922 - /opt/bleadperl/p4/perl/sv.c ====

Inline Patch

--- /home/rafael/tmp/tmp.19047.0        2005-06-20 23:39:44.024671048 +0200
+++ /opt/bleadperl/p4/perl/sv.c 2005-06-20 23:37:49.390098152 +0200
@@ -8847,7 +8847,7 @@ Perl_sv_vcatpvfn(pTHX_ SV *sv, const cha
 
 #ifndef USE_LONG_DOUBLE
     /* special-case "%.<number>[gf]" */
-    if ( patlen <= 5 && pat[0] == '%' && pat[1] == '.'
+    if ( !args && patlen <= 5 && pat[0] == '%' && pat[1] == '.'
         && (pat[patlen-1] == 'g' || pat[patlen-1] == 'f') ) {
        unsigned digits = 0;
        const char *pp;
@@ -8858,9 +8858,7 @@ Perl_sv_vcatpvfn(pTHX_ SV *sv, const cha
        if (pp - pat == (int)patlen - 1) {
            NV nv;
 
-           if (args)
-               nv = (NV)va_arg(*args, double);
-           else if (svix < svmax)
+           if (svix < svmax)
                nv = SvNV(*svargs);
            else
                return;

p5pRT · 2005-06-21T01:28:12Z

From @hvds

Rafael Garcia-Suarez <rgarciasuarez@gmail.com> wrote:
:On 6/17/05, hv wrote:
:> :Oops. I've overlooked the case 'g'. For the time being, I'll skip
:> :the block if args != null, removing va_arg() at all.
:>
:> Good, that's a much better solution than anything I suggested.
:
:I'm not 100% sure what you meant, is the following correct ? :
[snip]

Yes.

Hugo

p5pRT · 2005-06-21T07:30:45Z

From @rgs

hv@crypt.org wrote:

Rafael Garcia-Suarez <rgarciasuarez@gmail.com> wrote:
:On 6/17/05, hv wrote:
:> :Oops. I've overlooked the case 'g'. For the time being, I'll skip
:> :the block if args != null, removing va_arg() at all.
:>
:> Good, that's a much better solution than anything I suggested.
:
:I'm not 100% sure what you meant, is the following correct ? :
[snip]

Yes.

OK, I thus applied it to bleadperl as change #24916.

p5pRT · 2005-06-21T07:31:42Z

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

p5pRT · 2005-06-21T11:34:07Z

From t-ikegami@aist.go.jp

From: "Rafael Garcia-Suarez via RT" <perlbug-followup@perl.org>
Date: 20 Jun 2005 21:41:15 -0000

I'm not 100% sure what you meant, is the following correct ? :

That's what I'm using now.

truly,
IKEGAMI, Tsutomu
AIST
t-ikegami@aist.go.jp

p5pRT closed this as completed Jun 21, 2005

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sv_vcatpvfn() fails on some (very limited) situations #7976

sv_vcatpvfn() fails on some (very limited) situations #7976

p5pRT commented Jun 16, 2005

p5pRT commented Jun 16, 2005

p5pRT commented Jun 16, 2005

p5pRT commented Jun 16, 2005

p5pRT commented Jun 16, 2005

p5pRT commented Jun 16, 2005

p5pRT commented Jun 17, 2005

p5pRT commented Jun 17, 2005

p5pRT commented Jun 20, 2005

p5pRT commented Jun 21, 2005

p5pRT commented Jun 21, 2005

p5pRT commented Jun 21, 2005

p5pRT commented Jun 21, 2005

sv_vcatpvfn() fails on some (very limited) situations #7976

sv_vcatpvfn() fails on some (very limited) situations #7976

Comments

p5pRT commented Jun 16, 2005

p5pRT commented Jun 16, 2005

From ikegami@artemis.a01.aist.go.jp

p5pRT commented Jun 16, 2005

From @hvds

p5pRT commented Jun 16, 2005

p5pRT commented Jun 16, 2005

From @rgs

p5pRT commented Jun 16, 2005

From @hvds

p5pRT commented Jun 17, 2005

From t-ikegami@aist.go.jp

p5pRT commented Jun 17, 2005

From @hvds

p5pRT commented Jun 20, 2005

From @rgarcia

p5pRT commented Jun 21, 2005

From @hvds

p5pRT commented Jun 21, 2005

From @rgs

p5pRT commented Jun 21, 2005

p5pRT commented Jun 21, 2005

From t-ikegami@aist.go.jp