Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

syswrite prints garbage if called with empty scalar and non-zero offset #9804

Closed
p5pRT opened this issue Jul 27, 2009 · 10 comments
Closed

syswrite prints garbage if called with empty scalar and non-zero offset #9804

p5pRT opened this issue Jul 27, 2009 · 10 comments

Comments

@p5pRT
Copy link

@p5pRT p5pRT commented Jul 27, 2009

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

Searchable as RT67912$

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jul 27, 2009

From dave.taylor.cpan@googlemail.com

In a stock 5.10.0 perl with the default build options, syswrite prints
garbage if called with the empty string as the scalar and a non-zero
offset​:

$ /usr/local/refperl/5.10.0/bin/perl -e 'my $foo = ""; syswrite
STDOUT, $foo, 100, 1' | less
<DC>8 /null^@​^@​^@​^Y^@​^@​^@​^A^@​^@​^@​ ^@​^@​^@​^P^@​^@​^@​X^V9
^@​^@​^@​^@​<89>^@​^@​^@​<80><A6>^U^H^@​^@​^@​^@​^@​<A6>^U^H^@​^@​^@​^@​<80><A7>^U^H^@​^@​^@​^@​^@​
<A7>^U^H^@​^@​^@​^@​<80><A8>^U^H^@​^@​^@​^@​^@​<A9>^U^H^@​^@​^@​^@​^@​<A5>^U^H^@​^@​^@​^@​<80><A4>^U^H^@​
(END)

Loading

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jul 27, 2009

From dave.taylor.cpan@googlemail.com


Flags​:
  category=
  severity=


Site configuration information for perl 5.10.0​:

Configured by dave at Mon Jul 27 09​:34​:33 SAST 2009.

Summary of my perl5 (revision 5 version 10 subversion 0) configuration​:
  Platform​:
  osname=linux, osvers=2.6.27-14-generic, archname=i686-linux
  uname='linux pencil.home 2.6.27-14-generic #1 smp tue jun 30 19​:57​:39 utc 2009 i686 gnulinux '
  config_args='-de -Dprefix=/usr/local/refperl/5.10.0'
  hint=recommended, useposix=true, d_sigaction=define
  useithreads=undef, usemultiplicity=undef
  useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
  use64bitint=undef, use64bitall=undef, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='cc', ccflags ='-fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
  optimize='-O2',
  cppflags='-fno-strict-aliasing -pipe -I/usr/local/include'
  ccversion='', gccversion='4.3.2', 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 /usr/lib64
  libs=-lnsl -ldl -lm -lcrypt -lutil -lc
  perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
  libc=/lib/libc-2.8.90.so, so=so, useshrplib=false, libperl=libperl.a
  gnulibc_version='2.8.90'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
  cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib'

Locally applied patches​:
 


@​INC for perl 5.10.0​:
  /usr/local/refperl/5.10.0/lib/5.10.0/i686-linux
  /usr/local/refperl/5.10.0/lib/5.10.0
  /usr/local/refperl/5.10.0/lib/site_perl/5.10.0/i686-linux
  /usr/local/refperl/5.10.0/lib/site_perl/5.10.0
  .


Environment for perl 5.10.0​:
  HOME=/home/dave
  LANG=en_GB.UTF-8
  LANGUAGE (unset)
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)
  PATH=/usr/local/sbin​:/usr/local/bin​:/usr/sbin​:/usr/bin​:/sbin​:/bin​:/usr/games
  PERL_BADLANG (unset)
  SHELL=/bin/bash

Loading

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jul 27, 2009

From chromatic@wgz.org

On Monday 27 July 2009 01​:01​:32 David Taylor wrote​:

# New Ticket Created by David Taylor
# Please include the string​: [perl #67912]
# in the subject line of all future correspondence about this issue.
# <URL​: http​://rt.perl.org/rt3/Ticket/Display.html?id=67912 >

In a stock 5.10.0 perl with the default build options, syswrite prints
garbage if called with the empty string as the scalar and a non-zero
offset​:

$ /usr/local/refperl/5.10.0/bin/perl -e 'my $foo = ""; syswrite
STDOUT, $foo, 100, 1' | less
<DC>8 /null^@​^@​^@​^Y^@​^@​^@​^A^@​^@​^@​ ^@​^@​^@​^P^@​^@​^@​X^V9

^@​^@​^@​^@​<89>^@​^@​^@​<80><A6>^U^H^@​^@​^@​^@​^@​<A6>^U^H^@​^@​^@​^@​<80><A7>^U^H^@​^@​^@​^

@​^@​

<A7>^U^H^@​^@​^@​^@​<80><A8>^U^H^@​^@​^@​^@​^@​<A9>^U^H^@​^@​^@​^@​^@​<A5>^U^H^@​^@​^@​^@​<80

<A4>^U^H^@​ (END)

Confirmed in blead. This looks like a fencepost error. With the included
patch, I get instead an error​:

$ ./perl -e 'my $foo = ""; syswrite
STDOUT, $foo, 100, 1'
Offset outside string at -e line 1.

I suspect that the comparison should be > instead of >=, but it's late here
and another set of eyes would help.

Inline Patch
diff --git a/pp_sys.c b/pp_sys.c
index 23f79ba..ec12cd4 100644
--- a/pp_sys.c
+++ b/pp_sys.c
@@ -1919,7 +1919,7 @@ PP(pp_send)
                    DIE(aTHX_ "Offset outside string");
                }
                offset += blen_chars;
-           } else if (offset >= (IV)blen_chars && blen_chars > 0) {
+           } else if (offset >= (IV)blen_chars) {
                Safefree(tmpbuf);
                DIE(aTHX_ "Offset outside string");
            }

-- c

Loading

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jul 27, 2009

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

Loading

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jul 27, 2009

From offer.kaye@gmail.com

On Mon, Jul 27, 2009 at 2​:02 PM, chromatic wrote​:

Confirmed in blead.  This looks like a fencepost error.  With the included
patch, I get instead an error​:

Maybe we should add a test for this, perhaps in t/op/sysio.t? I
thought something like the attached patch (not tested).

Unrelated question - looking at t/op/sysio.t I don't understand, why
doesn't it use Test​::More?

Cheers,
--
Offer Kaye

Loading

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jul 27, 2009

From offer.kaye@gmail.com

0001-Add-test-for-perl-ticket-67912-syswrite-with-empty-s.patch
From 0ed53dcf1012f10c7acea14d04084bc3e1c582df Mon Sep 17 00:00:00 2001
From: Offer Kaye <offer@galaxy10.il.marvell.com>
Date: Mon, 27 Jul 2009 15:30:17 +0300
Subject: [PATCH] Add test for perl ticket #67912 - syswrite with empty string and non-zero offset.

---
 t/op/sysio.t |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/t/op/sysio.t b/t/op/sysio.t
index dd63a15..2ad41e4 100644
--- a/t/op/sysio.t
+++ b/t/op/sysio.t
@@ -1,6 +1,6 @@
 #!./perl
 
-print "1..42\n";
+print "1..43\n";
 
 chdir('op') || chdir('t/op') || die "sysio.t: cannot look for myself: $!";
 @INC = '../../lib';
@@ -240,6 +240,10 @@ unlink $outfile;
 
 chdir('..');
 
+eval { syswrite(STDOUT, '', 100, 1) };
+print 'not ' unless ($@ =~ /^Offset outside string/);
+print "ok 43\n";
+
 1;
 
 # eof
-- 
1.6.3.3

Loading

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jul 27, 2009

From perl@profvince.com

Confirmed in blead. This looks like a fencepost error. With the included
patch, I get instead an error​:

$ ./perl -e 'my $foo = ""; syswrite
STDOUT, $foo, 100, 1'
Offset outside string at -e line 1.

I suspect that the comparison should be > instead of >=, but it's late here
and another set of eyes would help.

diff --git a/pp_sys.c b/pp_sys.c
index 23f79ba..ec12cd4 100644
--- a/pp_sys.c
+++ b/pp_sys.c
@​@​ -1919,7 +1919,7 @​@​ PP(pp_send)
DIE(aTHX_ "Offset outside string");
}
offset += blen_chars;
- } else if (offset >= (IV)blen_chars && blen_chars > 0) {
+ } else if (offset >= (IV)blen_chars) {
Safefree(tmpbuf);
DIE(aTHX_ "Offset outside string");
}

-- c

I think that's the cause too, so I've applied it as
6100a0a and I added a regression test
with e41cc77.

But I don't think >= should be replaced by >, because an offset of n is
outside a string of length n.

Vincent.

Loading

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jul 27, 2009

bitcard@profvince.com - Status changed from 'open' to 'resolved'

Loading

@p5pRT p5pRT closed this Jul 27, 2009
@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jul 27, 2009

From zefram@fysh.org

Vincent Pit wrote​:

But I don't think >= should be replaced by >, because an offset of n is
outside a string of length n.

Offset of n is acceptable with a string of length n as long as you're not
intending to read more than zero chars from it. The proper conditions
for acceptability are​:

  write_length >= 0
  offset >= 0
  offset + write_length <= string_length

-zefram

Loading

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jul 28, 2009

From @nwc10

On Mon, Jul 27, 2009 at 03​:35​:02PM +0300, Offer Kaye wrote​:

On Mon, Jul 27, 2009 at 2​:02 PM, chromatic wrote​:

Confirmed in blead.  This looks like a fencepost error.  With the included
patch, I get instead an error​:

Maybe we should add a test for this, perhaps in t/op/sysio.t? I
thought something like the attached patch (not tested).

Unrelated question - looking at t/op/sysio.t I don't understand, why
doesn't it use Test​::More?

There's a bootstrapping order on tests. The initial tests run avoid relying
on complex constructions such as packages and OO, so that we get clear
failure diagnostics if we introduce a bug that makes them fail.

Nicholas Clark

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant