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

[PATCH] pp_syscall: "I32 retval" truncates the returned value #12248

Closed
p5pRT opened this issue Jul 4, 2012 · 17 comments
Closed

[PATCH] pp_syscall: "I32 retval" truncates the returned value #12248

p5pRT opened this issue Jul 4, 2012 · 17 comments

Comments

@p5pRT
Copy link

p5pRT commented Jul 4, 2012

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

Searchable as RT113980$

@p5pRT
Copy link
Author

p5pRT commented Jul 4, 2012

From oleg@redhat.com

Hello,

I noticed today that syscall(9, ...) (mmap) doesn't work for me.

The problem is obvious, pp_syscall() uses I32 for retval and the
"long" address doesn't fit into "int".

The one-liner below should fix the problem. I am not sure where
should I send this patch, how should I write the changelog, etc.
IOW, sorry if my email doesn't conform the rules.

And btw, thanks to all perl developers for perl ;)

Oleg.

--- a/pp_sys.c
+++ b/pp_sys.c
@​@​ -5456,7 +5456,7 @​@​ PP(pp_syscall)
  register I32 items = SP - MARK;
  unsigned long a[20];
  register I32 i = 0;
- I32 retval = -1;
+ IV retval = -1;

  if (PL_tainting) {
  while (++MARK <= SP) {

@p5pRT
Copy link
Author

p5pRT commented Jul 4, 2012

From @cpansprout

On Wed Jul 04 05​:56​:04 2012, oleg@​redhat.com wrote​:

Hello,

I noticed today that syscall(9, ...) (mmap) doesn't work for me.

The problem is obvious, pp_syscall() uses I32 for retval and the
"long" address doesn't fit into "int".

The one-liner below should fix the problem. I am not sure where
should I send this patch, how should I write the changelog, etc.
IOW, sorry if my email doesn't conform the rules.

And btw, thanks to all perl developers for perl ;)

Oleg.

--- a/pp_sys.c
+++ b/pp_sys.c
@​@​ -5456,7 +5456,7 @​@​ PP(pp_syscall)
register I32 items = SP - MARK;
unsigned long a[20];
register I32 i = 0;
- I32 retval = -1;
+ IV retval = -1;

 if \(PL\_tainting\) \{
 while \(\+\+MARK \<= SP\) \{

Thank you. Applied as f9344c9. I don’t supposed it’s possible to
write an automated test for this, is it?

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jul 4, 2012

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

@p5pRT
Copy link
Author

p5pRT commented Jul 4, 2012

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

@p5pRT p5pRT closed this as completed Jul 4, 2012
@p5pRT
Copy link
Author

p5pRT commented Jul 5, 2012

From blgl@stacken.kth.se

Quoth Oleg Nesterov​:

--- a/pp_sys.c
+++ b/pp_sys.c
@​@​ -5456,7 +5456,7 @​@​ PP(pp_syscall)
register I32 items = SP - MARK;
unsigned long a[20];
register I32 i = 0;
- I32 retval = -1;
+ IV retval = -1;

if (PL_tainting) {
while (++MARK <= SP) {

Note that this doesn't help if the syscall function itself is declared as

  int syscall(int, ...);

which is not unheard of.

/Bo Lindbergh

@p5pRT
Copy link
Author

p5pRT commented Jul 5, 2012

From oleg@redhat.com

Father,

I do not know if it is OK to reply to perlbug-followup@​perl.org,
so I CC'ed you explicitly.

On 07/04, Father Chrysostomos via RT wrote​:

I don’t supposed it’s possible to
write an automated test for this, is it?

Well, please see below. Not sure it is the best...

So. The test-case assumes 64-bit linux, otherwise it won't work.

It exploits the fact that vdso is mmapped at "high" address which
can't fit in I32.

It does mremap(vdso_addr, 1, 1, 0) which is "nop" but should return
the original address.

Btw, "-w" complains​:

  Hexadecimal number > 0xffffffff non-portable at ...

a bit annoying on 64-bit systems... Not sure why Perl_grok_hex()
thinks this is not portable.

Oleg.


#!/usr/bin/perl -w
use strict;

sub find_vdso()
{
  open my $maps, '<', '/proc/self/maps' or die;
  /^(.*?)-.*\[vdso\]$/ and return hex $1 for <$maps>;
}

my $vdso_addr = find_vdso || die 'not 64-bit linux?';

my $ret = syscall 25, # __NR_mremap
  $vdso_addr, # old_addr
  1, # old_len
  1, # new_len
  0, # flags
  0; # new_addr (unused)

$ret == $vdso_addr or die "FAILED\n";

@p5pRT
Copy link
Author

p5pRT commented Jul 6, 2012

From oleg@redhat.com

On 07/04, Bo Lindbergh via RT wrote​:

Quoth Oleg Nesterov​:

--- a/pp_sys.c
+++ b/pp_sys.c
@​@​ -5456,7 +5456,7 @​@​ PP(pp_syscall)
register I32 items = SP - MARK;
unsigned long a[20];
register I32 i = 0;
- I32 retval = -1;
+ IV retval = -1;

if (PL_tainting) {
while (++MARK <= SP) {

Note that this doesn't help if the syscall function itself is declared as

int syscall(int, ...);

which is not unheard of.

Sure, this change is not needed in this case, but it doesn't hurt?
I am not familiar with perl's implementation but PUSHi(retval) and
Perl_sv_setiv() uses (IV)retval anyway, so I don't think this change
can make any difference.

If you meant that libc itself can be buggy (so that the library
function truncates the result of syscall), then perl can do nothing.

Or I misunderstood your point?

Oleg.

@p5pRT
Copy link
Author

p5pRT commented Jul 9, 2012

From @cpansprout

On Thu Jul 05 02​:15​:16 2012, oleg@​redhat.com wrote​:

Father,

I do not know if it is OK to reply to perlbug-followup@​perl.org,
so I CC'ed you explicitly.

On 07/04, Father Chrysostomos via RT wrote​:

I don’t supposed it’s possible to
write an automated test for this, is it?

Well, please see below. Not sure it is the best...

So. The test-case assumes 64-bit linux, otherwise it won't work.

It exploits the fact that vdso is mmapped at "high" address which
can't fit in I32.

It does mremap(vdso_addr, 1, 1, 0) which is "nop" but should return
the original address.

Btw, "-w" complains​:

Hexadecimal number > 0xffffffff non\-portable at \.\.\.

a bit annoying on 64-bit systems... Not sure why Perl_grok_hex()
thinks this is not portable.

Oleg.

-------------------------------------------------------------------
#!/usr/bin/perl -w
use strict;

sub find_vdso()
{
open my $maps, '<', '/proc/self/maps' or die;
/^(.*?)-.*\[vdso\]$/ and return hex $1 for <$maps>;
}

my $vdso_addr = find_vdso || die 'not 64-bit linux?';

my $ret = syscall 25, # __NR_mremap
$vdso_addr, # old_addr
1, # old_len
1, # new_len
0, # flags
0; # new_addr (unused)

$ret == $vdso_addr or die "FAILED\n";

Thank you. I’ll have to adjust this a bit to turn it into a test. Does
$^O give you ‘linux’?

There is a possibility that another OS might have a plain file called
/proc/self/maps whose contents just happen to match the regular
expression, resulting in an invalid syscall, and perhaps a crash.

So we should skip early if $^O is not appropriate.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jul 11, 2012

From oleg@redhat.com

On 07/08, Father Chrysostomos via RT wrote​:

On Thu Jul 05 02​:15​:16 2012, oleg@​redhat.com wrote​:

-------------------------------------------------------------------
#!/usr/bin/perl -w
use strict;

sub find_vdso()
{
open my $maps, '<', '/proc/self/maps' or die;
/^(.*?)-.*\[vdso\]$/ and return hex $1 for <$maps>;
}

my $vdso_addr = find_vdso || die 'not 64-bit linux?';

my $ret = syscall 25, # __NR_mremap
$vdso_addr, # old_addr
1, # old_len
1, # new_len
0, # flags
0; # new_addr (unused)

$ret == $vdso_addr or die "FAILED\n";

Thank you. I’ll have to adjust this a bit to turn it into a test. Does
$^O give you ‘linux’?

Yes,

There is a possibility that another OS might have a plain file called
/proc/self/maps whose contents just happen to match the regular
expression, resulting in an invalid syscall, and perhaps a crash.

So we should skip early if $^O is not appropriate.

Yes, yes, sure.

Bu just in case, please note $^O eq 'linux' is not enough.

The test-case assumes 64-bit linux and perl should be compiled
as 64-bit application too. Otherwise syscall(25) means stime()
and should be skipped.

Oleg.

@p5pRT
Copy link
Author

p5pRT commented Jul 26, 2012

From @cpansprout

On Wed Jul 11 03​:57​:53 2012, oleg@​redhat.com wrote​:

On 07/08, Father Chrysostomos via RT wrote​:

On Thu Jul 05 02​:15​:16 2012, oleg@​redhat.com wrote​:

-------------------------------------------------------------------
#!/usr/bin/perl -w
use strict;

sub find_vdso()
{
open my $maps, '<', '/proc/self/maps' or die;
/^(.*?)-.*\[vdso\]$/ and return hex $1 for <$maps>;
}

my $vdso_addr = find_vdso || die 'not 64-bit linux?';

my $ret = syscall 25, # __NR_mremap
$vdso_addr, # old_addr
1, # old_len
1, # new_len
0, # flags
0; # new_addr (unused)

$ret == $vdso_addr or die "FAILED\n";

Thank you. I’ll have to adjust this a bit to turn it into a test. Does
$^O give you ‘linux’?

Yes,

There is a possibility that another OS might have a plain file called
/proc/self/maps whose contents just happen to match the regular
expression, resulting in an invalid syscall, and perhaps a crash.

So we should skip early if $^O is not appropriate.

Yes, yes, sure.

Bu just in case, please note $^O eq 'linux' is not enough.

The test-case assumes 64-bit linux and perl should be compiled
as 64-bit application too. Otherwise syscall(25) means stime()
and should be skipped.

This line accounts for that, does it not?

  my $vdso_addr = find_vdso || die 'not 64-bit linux?';

Please try out the attached patch, based on your script. If it works
for you, I’ll apply it.

I don’t have access to a 32-bit linux system, so I am unable to test the
skip. Are you?

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jul 26, 2012

From @cpansprout

From 9640580f6072df9034acabf7e8f167839751bccd Mon Sep 17 00​:00​:00 2001
From​: Father Chrysostomos <sprout@​cpan.org>
Date​: Wed, 25 Jul 2012 23​:03​:16 -0700
Subject​: [PATCH] Test for #113980 (syscall retval truncation)

Based on a script provided by Oleg Nesterov.

Inline Patch
diff --git a/MANIFEST b/MANIFEST
index 18088ca..541b4d9 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -5340,6 +5340,7 @@ t/op/sub.t			See if subroutines work
 t/op/svleak.t			See if stuff leaks SVs
 t/op/switch.t			See if switches (given/when) work
 t/op/symbolcache.t		See if undef/delete works on stashes with functions
+t/op/syscall-113980.t		Test that bug #113980 is fixed
 t/op/sysio.t			See if sysread and syswrite work
 t/op/taint.t			See if tainting works
 t/op/threads_create.pl		Ancillary file for t/op/threads.t
diff --git a/t/op/syscall-113980.t b/t/op/syscall-113980.t
new file mode 100644
index 0000000..342997b
--- /dev/null
+++ b/t/op/syscall-113980.t
@@ -0,0 +1,26 @@
+#!/usr/bin/perl -w
+BEGIN { chdir 't'; require './test.pl' }
+use strict;
+
+skip_all "not linux" unless $^O eq 'linux';
+
+sub find_vdso()
+{
+	open my $maps, '<', '/proc/self/maps'
+	    or skip_all "/proc/self/maps: $!";
+	no warnings 'portable';
+	/^(.*?)-.*\[vdso\]$/ and return hex $1 for <$maps>;
+}
+
+my $vdso_addr = find_vdso || skip_all 'not 64-bit linux?';
+
+plan 1;
+
+my $ret = syscall 25,		# __NR_mremap
+		$vdso_addr,	# old_addr
+		1,		# old_len
+		1,		# new_len
+		0,		# flags
+		0;		# new_addr (unused)
+
+is $ret, $vdso_addr, "syscall retval is not truncated";

@p5pRT
Copy link
Author

p5pRT commented Jul 26, 2012

From @nwc10

On Wed, Jul 25, 2012 at 11​:15​:01PM -0700, Father Chrysostomos via RT wrote​:

On Wed Jul 11 03​:57​:53 2012, oleg@​redhat.com wrote​:

The test-case assumes 64-bit linux and perl should be compiled
as 64-bit application too. Otherwise syscall(25) means stime()
and should be skipped.

This line accounts for that, does it not?

my $vdso\_addr = find\_vdso || die 'not 64\-bit linux?';

Please try out the attached patch, based on your script. If it works
for you, I'll apply it.

I don't have access to a 32-bit linux system, so I am unable to test the
skip. Are you?

I get this failure on an x86 system​:

$ ./perl -I../lib op/syscall-113980.t
1..1
not ok 1 - syscall retval is not truncated
# Failed test 1 - syscall retval is not truncated at op/syscall-113980.t line 26
# got "-1"
# expected "3077869568"

for information​:

$ cat /proc/self/maps
08048000-08052000 r-xp 00000000 ca​:01 131074 /bin/cat
08052000-08053000 rw-p 0000a000 ca​:01 131074 /bin/cat
08de5000-08e06000 rw-p 00000000 00​:00 0 [heap]
b75c4000-b75c5000 rw-p 00000000 00​:00 0
b75c5000-b7705000 r-xp 00000000 ca​:01 1647095 /lib/i686/cmov/libc-2.11.3.so
b7705000-b7706000 ---p 00140000 ca​:01 1647095 /lib/i686/cmov/libc-2.11.3.so
b7706000-b7708000 r--p 00140000 ca​:01 1647095 /lib/i686/cmov/libc-2.11.3.so
b7708000-b7709000 rw-p 00142000 ca​:01 1647095 /lib/i686/cmov/libc-2.11.3.so
b7709000-b770c000 rw-p 00000000 00​:00 0
b7718000-b771c000 rw-p 00000000 00​:00 0
b771c000-b771d000 r-xp 00000000 00​:00 0 [vdso]
b771d000-b7738000 r-xp 00000000 ca​:01 1630731 /lib/ld-2.11.3.so
b7738000-b7739000 r--p 0001b000 ca​:01 1630731 /lib/ld-2.11.3.so
b7739000-b773a000 rw-p 0001c000 ca​:01 1630731 /lib/ld-2.11.3.so
bfbda000-bfbef000 rw-p 00000000 00​:00 0 [stack]

The sparc linux build is still ongoing...
(I think this is a 32 build on a sparc64 system. The machine is slow, and I'm
not going to interrupt it to find out out for sure)

I've not yet tried coaxing a build with -m32 on x86_64

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Jul 26, 2012

From @cpansprout

On Thu Jul 26 09​:19​:42 2012, nicholas wrote​:

On Wed, Jul 25, 2012 at 11​:15​:01PM -0700, Father Chrysostomos via RT
wrote​:

On Wed Jul 11 03​:57​:53 2012, oleg@​redhat.com wrote​:

The test-case assumes 64-bit linux and perl should be compiled
as 64-bit application too. Otherwise syscall(25) means stime()
and should be skipped.

This line accounts for that, does it not?

my $vdso\_addr = find\_vdso || die 'not 64\-bit linux?';

Please try out the attached patch, based on your script. If it
works
for you, I'll apply it.

I don't have access to a 32-bit linux system, so I am unable to test
the
skip. Are you?

I get this failure on an x86 system​:

$ ./perl -I../lib op/syscall-113980.t
1..1
not ok 1 - syscall retval is not truncated
# Failed test 1 - syscall retval is not truncated at op/syscall-
113980.t line 26
# got "-1"
# expected "3077869568"

for information​:

$ cat /proc/self/maps
08048000-08052000 r-xp 00000000 ca​:01 131074 /bin/cat
08052000-08053000 rw-p 0000a000 ca​:01 131074 /bin/cat
08de5000-08e06000 rw-p 00000000 00​:00 0 [heap]
b75c4000-b75c5000 rw-p 00000000 00​:00 0
b75c5000-b7705000 r-xp 00000000 ca​:01 1647095 /lib/i686/cmov/libc-
2.11.3.so
b7705000-b7706000 ---p 00140000 ca​:01 1647095 /lib/i686/cmov/libc-
2.11.3.so
b7706000-b7708000 r--p 00140000 ca​:01 1647095 /lib/i686/cmov/libc-
2.11.3.so
b7708000-b7709000 rw-p 00142000 ca​:01 1647095 /lib/i686/cmov/libc-
2.11.3.so
b7709000-b770c000 rw-p 00000000 00​:00 0
b7718000-b771c000 rw-p 00000000 00​:00 0
b771c000-b771d000 r-xp 00000000 00​:00 0 [vdso]
b771d000-b7738000 r-xp 00000000 ca​:01 1630731 /lib/ld-2.11.3.so
b7738000-b7739000 r--p 0001b000 ca​:01 1630731 /lib/ld-2.11.3.so
b7739000-b773a000 rw-p 0001c000 ca​:01 1630731 /lib/ld-2.11.3.so
bfbda000-bfbef000 rw-p 00000000 00​:00 0 [stack]

The sparc linux build is still ongoing...
(I think this is a 32 build on a sparc64 system. The machine is slow,
and I'm
not going to interrupt it to find out out for sure)

I've not yet tried coaxing a build with -m32 on x86_64

Thank you. That means the skip is not being triggered. Do you know how
to test that this is a 64-bit linux system? Or is this bug something we
just can’t reliably test?

Obviously calling stime is no good, even if it only works under root.
Running tests under root should not set the system time. :-)

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jul 26, 2012

From @nwc10

On Thu, Jul 26, 2012 at 09​:59​:49AM -0700, Father Chrysostomos via RT wrote​:

On Thu Jul 26 09​:19​:42 2012, nicholas wrote​:

The sparc linux build is still ongoing...
(I think this is a 32 build on a sparc64 system. The machine is slow,
and I'm
not going to interrupt it to find out out for sure)

However, the 4 core MIPs box is faster. And on it,

$ ./perl -I../lib op/syscall-113980.t
1..1
not ok 1 - syscall retval is not truncated
# Failed test 1 - syscall retval is not truncated at op/syscall-113980.t line 26
# got "-1"
# expected "2147434496"

I've not yet tried coaxing a build with -m32 on x86_64

... -Accflags=-m32 -Aldflags=-m32 -Alddlflags=-m32\ -shared

[and I don't know why I need to add -shared - is the hints file buggy?]

$ ./perl -I../lib op/syscall-113980.t
1..1
not ok 1 - syscall retval is not truncated
# Failed test 1 - syscall retval is not truncated at op/syscall-113980.t line 26
# got "-1"
# expected "4160614400"

(output of ./perl -pe0 /proc/self/maps >maps attached)

Thank you. That means the skip is not being triggered. Do you know how
to test that this is a 64-bit linux system? Or is this bug something we
just can't reliably test?

No, I don't for sure. I think if one is sure it's Linux, run uname -m

eg

$ uname -m
x86_64
$ uname -m
mips64
$ uname -m
sparc64

vs

$ uname -m
i686
$ uname -m
armv6l

I think all the 64 bit architectures happen to match /64$/
However, I suspect that syscall numbers vary with architecture, and arguments
might too, so it's not going to be safe to use a hardcoded magic number
even on systems which seem to be 64 bits - really, check if it's exactly
x86_64. This also assumes that syscall numbers do not change on Linux.
I guess that they can't now, given that 3.x.y is really just 2.6.z, but I
presume that they can be deprecated and eventually dropped.

But as the above demonstrates, it would need to be a 64 bit process on a 64
bit OS. I think the latter is fairly easy - $Config{ptrsize}.

Obviously calling stime is no good, even if it only works under root.
Running tests under root should not set the system time. :-)

For starters, I think skip it if running as root.
But yes, also skip unless it's linux, skip unless it's x86_64, and skip
unless it's a 64 bit build.

But I'm not confident how safe or reliable this sort of thing is ever going
to be.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Jul 26, 2012

From @cpansprout

On Thu Jul 26 12​:03​:15 2012, nicholas wrote​:

For starters, I think skip it if running as root.
But yes, also skip unless it's linux, skip unless it's x86_64, and
skip
unless it's a 64 bit build.

But I'm not confident how safe or reliable this sort of thing is ever
going
to be.

I think we just have to leave this untested.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jul 26, 2012

From [Unknown Contact. See original ticket]

On Thu Jul 26 12​:03​:15 2012, nicholas wrote​:

For starters, I think skip it if running as root.
But yes, also skip unless it's linux, skip unless it's x86_64, and
skip
unless it's a 64 bit build.

But I'm not confident how safe or reliable this sort of thing is ever
going
to be.

I think we just have to leave this untested.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jul 30, 2012

From @nwc10

On Thu, Jul 26, 2012 at 12​:18​:01PM -0700, Father Chrysostomos via RT wrote​:

On Thu Jul 26 12​:03​:15 2012, nicholas wrote​:

For starters, I think skip it if running as root.
But yes, also skip unless it's linux, skip unless it's x86_64, and
skip
unless it's a 64 bit build.

But I'm not confident how safe or reliable this sort of thing is ever
going
to be.

I think we just have to leave this untested.

Yes, I suspect so too.

Nicholas Clark

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

No branches or pull requests

1 participant