From: Dr. D. A. G. <da...@tr...> - 2013-11-04 22:23:16
|
The 'trinity' fuzz tester managed to trigger a seg of strace when doing a select() with a -ve nfds value but pointing to a valid large buffer (on x86 Linux). My patch below fixes this; I'm not 100% happy because: 1) It seems way too complicated - can't we quit earlier when we find the length is weird? 2) It's odd the way the code reads the arg for fdsize and then later reads it again for nfds, I think that's really the underlying reason this tripped. 3) I'd like some reassurance that my understanding of the way strace's arg types work is right. (WTH does strace use int for nfds?) Thoughts? Dave commit 9354b400e243cc87f8b2964c4220afcbd077fb30 Author: Dr. David Alan Gilbert <da...@tr...> Date: Mon Nov 4 21:45:24 2013 +0000 Fix select() with -ve nfds select() with a -ve nfds value but a valid buffer seg'd. Work with the nfds parameters as an int everywhere (which it is) and flag when we fixup the value so that we don't then try and walk the buffer. Also adds test/select.c to capture the case. diff --git a/desc.c b/desc.c index 9bfe4d0..30ff7a7 100644 --- a/desc.c +++ b/desc.c @@ -481,24 +481,29 @@ static int decode_select(struct tcb *tcp, long *args, enum bitness_t bitness) { int i, j; - unsigned nfds, fdsize; + int nfds, fdsize; fd_set *fds; const char *sep; long arg; + int unusual_nfds = 0; - fdsize = args[0]; + fdsize = (int)args[0]; /* Beware of select(2^31-1, NULL, NULL, NULL) and similar... */ - if (args[0] > 1024*1024) + if (fdsize > 1024*1024) { fdsize = 1024*1024; - if (args[0] < 0) + unusual_nfds = 1; + } + if (fdsize < 0) { fdsize = 0; + unusual_nfds = 1; + } fdsize = (((fdsize + 7) / 8) + sizeof(long)-1) & -sizeof(long); if (entering(tcp)) { fds = malloc(fdsize); if (!fds) die_out_of_memory(); - nfds = args[0]; + nfds = (int)args[0]; tprintf("%d", nfds); for (i = 0; i < 3; i++) { arg = args[i+1]; @@ -510,7 +515,8 @@ decode_select(struct tcb *tcp, long *args, enum bitness_t bitness) tprintf(", %#lx", arg); continue; } - if (umoven(tcp, arg, fdsize, (char *) fds) < 0) { + if ((umoven(tcp, arg, fdsize, (char *) fds) < 0) || + unusual_nfds) { tprints(", [?]"); continue; } diff --git a/test/.gitignore b/test/.gitignore index 7eb39cf..c73b64a 100644 --- a/test/.gitignore +++ b/test/.gitignore @@ -10,4 +10,5 @@ wait_must_be_interruptible threaded_execve mtd ubi +select sigreturn diff --git a/test/Makefile b/test/Makefile index 92142b1..cc7d47a 100644 --- a/test/Makefile +++ b/test/Makefile @@ -3,7 +3,7 @@ CFLAGS += -Wall PROGS = \ vfork fork sig skodic clone leaderkill childthread \ sigkill_rain wait_must_be_interruptible threaded_execve \ - mtd ubi sigreturn + mtd ubi select sigreturn all: $(PROGS) diff --git a/test/select.c b/test/select.c new file mode 100644 index 0000000..fba54b3 --- /dev/null +++ b/test/select.c @@ -0,0 +1,31 @@ +/* da...@tr... */ + +#include <sys/select.h> + +#include <sys/time.h> +#include <sys/types.h> + +#include <stdlib.h> +#include <string.h> +#include <unistd.h> + +#define BUFSIZE 1024*1024*2 +char buffer[BUFSIZE]; + +int main () +{ + fd_set rds; + + FD_ZERO(&rds); + + FD_SET(2,&rds); + /* Start with a nice simple select */ + select(3, &rds, &rds, &rds, NULL); + + /* Now the crash case that trinity found, -ve nfds + but with a pointer to a large chunk of valid memory */ + select(-1, (fd_set *)buffer, NULL, NULL,NULL); + + exit(0); +} + -- -----Open up your eyes, open up your mind, open up your code ------- / Dr. David Alan Gilbert | Running GNU/Linux | Happy \ \ gro.gilbert @ treblig.org | | In Hex / \ _________________________|_____ http://www.treblig.org |_______/ |
From: Denys V. <dvl...@re...> - 2013-11-05 11:08:58
|
On 11/04/2013 10:39 PM, Dr. David Alan Gilbert wrote: > The 'trinity' fuzz tester managed to trigger a seg of strace > when doing a select() with a -ve nfds value but pointing to a valid large > buffer (on x86 Linux). > > My patch below fixes this; I'm not 100% happy because: > 1) It seems way too complicated - can't we quit earlier when we find > the length is weird? Yes, it can be simpler. > 2) It's odd the way the code reads the arg for fdsize and then later > reads it again for nfds, I think that's really the underlying > reason this tripped. I propose to do simply this: + nfds = fdsize; fdsize = (((fdsize + 7) / 8) + sizeof(long)-1) & -sizeof(long); + /* We had bugs a-la "while (j < args[0])" and "umoven(args[0])" below. + * Instead of args[0], use nfds for fd count, fdsize for array lengths. + */ and use nfds in those two places where we incorrectly use arg[0] now. > 3) I'd like some reassurance that my understanding of the way > strace's arg types work is right. > > (WTH does strace use int for nfds?) Because having more than 2^31-1 file descriptors in one process is insanity. > Thoughts? I applied a slightly simplified version of your fix to strace git, please try it. -- vda |
From: Dr. D. A. G. <da...@tr...> - 2013-11-05 12:49:18
|
* Denys Vlasenko (dvl...@re...) wrote: > On 11/04/2013 10:39 PM, Dr. David Alan Gilbert wrote: Hi Denys, Thanks for the reply, > > The 'trinity' fuzz tester managed to trigger a seg of strace > > when doing a select() with a -ve nfds value but pointing to a valid large > > buffer (on x86 Linux). > > > > My patch below fixes this; I'm not 100% happy because: > > 1) It seems way too complicated - can't we quit earlier when we find > > the length is weird? > > Yes, it can be simpler. > > > 2) It's odd the way the code reads the arg for fdsize and then later > > reads it again for nfds, I think that's really the underlying > > reason this tripped. > > I propose to do simply this: > > + nfds = fdsize; > fdsize = (((fdsize + 7) / 8) + sizeof(long)-1) & -sizeof(long); > + /* We had bugs a-la "while (j < args[0])" and "umoven(args[0])" below. > + * Instead of args[0], use nfds for fd count, fdsize for array lengths. > + */ > > and use nfds in those two places where we incorrectly use arg[0] now. > > 3) I'd like some reassurance that my understanding of the way > > strace's arg types work is right. > > > > (WTH does strace use int for nfds?) > > Because having more than 2^31-1 file descriptors in one process is insanity. Only twice as insane as having 2^30-1 file descriptors! > > Thoughts? > > I applied a slightly simplified version of your fix to strace git, please try it. That still fails (this is FORTIFY detecting the fail). I can see two things potentially; the simple one is that the types are still wrong so that the -1 starts out as 2^32-1 and goes through the route of being corrected to be 1024*1024 rather than 0. However, the other thing I think is being caught here is that fortify is catching FD_ISSET on a value greater than the size of the fd_set, so the added test below causes it to hit that failure - I guess the only solution there is not to use FD_ISSET or to cap at max-fds rather than the arbitrary 1024*1024 ? (I guess you could argue that's a false positive from fortify, but there again I think it is an illegal use of FD_ISSET). Anyway, here is the signedness fix: diff --git a/desc.c b/desc.c index 384b147..92cdf48 100644 --- a/desc.c +++ b/desc.c @@ -481,16 +481,16 @@ static int decode_select(struct tcb *tcp, long *args, enum bitness_t bitness) { int i, j; - unsigned nfds, fdsize; + int nfds, fdsize; fd_set *fds; const char *sep; long arg; - fdsize = args[0]; + fdsize = (int)args[0]; /* Beware of select(2^31-1, NULL, NULL, NULL) and similar... */ - if (args[0] > 1024*1024) + if (fdsize > 1024*1024) fdsize = 1024*1024; - if (args[0] < 0) + if (fdsize < 0) fdsize = 0; nfds = fdsize; fdsize = (((fdsize + 7) / 8) + sizeof(long)-1) & -sizeof(long); @@ -502,7 +502,7 @@ decode_select(struct tcb *tcp, long *args, enum bitness_t bitness) fds = malloc(fdsize); if (!fds) die_out_of_memory(); - tprintf("%ld", args[0]); + tprintf("%d", (int)args[0]); for (i = 0; i < 3; i++) { arg = args[i+1]; if (arg == 0) { diff --git a/test/select.c b/test/select.c index aee9f43..09f29c9 100644 --- a/test/select.c +++ b/test/select.c @@ -11,6 +11,8 @@ char buffer[1024*1024*2]; int main() { fd_set rds; + struct timeval timeout; + FD_ZERO(&rds); FD_SET(2, &rds); /* Start with a nice simple select */ @@ -18,6 +20,15 @@ int main() /* Now the crash case that trinity found, negative nfds * but with a pointer to a large chunk of valid memory. */ + FD_ZERO((fd_set*)buffer); + FD_SET(2,(fd_set*)buffer); select(-1, (fd_set *)buffer, NULL, NULL, NULL); + + /* Another variant, a large +ve value larger than + * the allowed number of fds. + */ + timeout.tv_sec = 0; + timeout.tv_usec = 100; + select(10000, (fd_set *)buffer, NULL, NULL, &timeout); return 0; } -- -----Open up your eyes, open up your mind, open up your code ------- / Dr. David Alan Gilbert | Running GNU/Linux | Happy \ \ gro.gilbert @ treblig.org | | In Hex / \ _________________________|_____ http://www.treblig.org |_______/ |
From: Denys V. <dvl...@re...> - 2013-11-05 15:19:45
|
On 11/05/2013 01:33 PM, Dr. David Alan Gilbert wrote: > * Denys Vlasenko (dvl...@re...) wrote: >> I propose to do simply this: >> >> + nfds = fdsize; >> fdsize = (((fdsize + 7) / 8) + sizeof(long)-1) & -sizeof(long); >> + /* We had bugs a-la "while (j < args[0])" and "umoven(args[0])" below. >> + * Instead of args[0], use nfds for fd count, fdsize for array lengths. >> + */ >> >> and use nfds in those two places where we incorrectly use arg[0] now. > >>> Thoughts? >> >> I applied a slightly simplified version of your fix to strace git, please try it. > > That still fails (this is FORTIFY detecting the fail). Please elaborate. You get a warning about access to fd_set->[__]fds_bits array past its declared bounds? How it looks? Does strace abort or what? > I can see two things potentially; the simple one is that the types are still > wrong so that the -1 starts out as 2^32-1 and goes through the route of being > corrected to be 1024*1024 rather than 0. Okay, let's do this. > However, the other thing I think is being caught here is that fortify is > catching FD_ISSET on a value greater than the size of the fd_set, > so the added test below causes it to hit that failure - I guess the > only solution there is not to use FD_ISSET or to cap at max-fds rather than > the arbitrary 1024*1024 ? There can be legitimate programs which use select() in excess of glibc-imposed artificial limit on bit array sizes. > (I guess you could argue that's a false positive from fortify, but there > again I think it is an illegal use of FD_ISSET). Do you see a reasonably portable way to check FD_ISSET? On the related note, how are we doing in "stracing 32-bit app with 64-bit strace on a big-endian machine" case? In that case, sizeof(long) is important... I dread to think about that:( -- vda |
From: Dr. D. A. G. <da...@tr...> - 2013-11-05 18:43:02
|
* Denys Vlasenko (dvl...@re...) wrote: > On 11/05/2013 01:33 PM, Dr. David Alan Gilbert wrote: > > That still fails (this is FORTIFY detecting the fail). > > Please elaborate. You get a warning about access to fd_set->[__]fds_bits > array past its declared bounds? How it looks? Does strace abort or what? $ ./strace test/select ..... select(3, [2], [2], [2], NULL) = 1 () select(-1, [], NULL, NULL, NULL) = -1 EINVAL (Invalid argument) *** buffer overflow detected ***: /home/dg/git/strace-code/./strace terminated ...... select(10000, [2 Program received signal SIGABRT, Aborted. 0x00007ffff7a48f77 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 56 ../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory. (gdb) where #0 0x00007ffff7a48f77 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 0x00007ffff7a4c5e8 in __GI_abort () at abort.c:90 #2 0x00007ffff7a864fb in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0x7ffff7b97f10 "*** %s ***: %s terminated\n") at ../sysdeps/unix/sysv/linux/libc_fatal.c:199 #3 0x00007ffff7b2408c in __GI___fortify_fail (msg=<optimised out>, msg@entry=0x7ffff7b97ea7 "buffer overflow detected") at fortify_fail.c:37 #4 0x00007ffff7b23020 in __GI___chk_fail () at chk_fail.c:28 #5 0x00007ffff7b23fd7 in __fdelt_chk (d=d@entry=1024) at fdelt_chk.c:25 #6 0x0000000000403b19 in decode_select (tcp=0x66b030, args=0x66b048, bitness=BITNESS_CURRENT) at desc.c:522 #7 0x0000000000415b88 in trace_syscall_entering (tcp=0x66b030) at syscall.c:2050 #8 trace_syscall (tcp=tcp@entry=0x66b030) at syscall.c:2722 #9 0x0000000000413f50 in trace () at strace.c:2332 #10 0x00000000004023af in main (argc=<optimised out>, argv=<optimised out>) at strace.c:2362 (gdb) up #1 0x00007ffff7a4c5e8 in __GI_abort () at abort.c:90 90 abort.c: No such file or directory. (gdb) #2 0x00007ffff7a864fb in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0x7ffff7b97f10 "*** %s ***: %s terminated\n") at ../sysdeps/unix/sysv/linux/libc_fatal.c:199 199 ../sysdeps/unix/sysv/linux/libc_fatal.c: No such file or directory. (gdb) #3 0x00007ffff7b2408c in __GI___fortify_fail (msg=<optimised out>, msg@entry=0x7ffff7b97ea7 "buffer overflow detected") at fortify_fail.c:37 37 fortify_fail.c: No such file or directory. (gdb) #4 0x00007ffff7b23020 in __GI___chk_fail () at chk_fail.c:28 28 chk_fail.c: No such file or directory. (gdb) #5 0x00007ffff7b23fd7 in __fdelt_chk (d=d@entry=1024) at fdelt_chk.c:25 25 fdelt_chk.c: No such file or directory. (gdb) #6 0x0000000000403b19 in decode_select (tcp=0x66b030, args=0x66b048, bitness=BITNESS_CURRENT) at desc.c:522 522 if (FD_ISSET(j, fds)) { So it does look like the FD_ISSET is blowing up. (For ref this is an Ubuntu Trusty x86-64 linux VM - but I don't see anything specific about it other than the fact they have fortify on) > > I can see two things potentially; the simple one is that the types are still > > wrong so that the -1 starts out as 2^32-1 and goes through the route of being > > corrected to be 1024*1024 rather than 0. > > Okay, let's do this. > > > However, the other thing I think is being caught here is that fortify is > > catching FD_ISSET on a value greater than the size of the fd_set, > > so the added test below causes it to hit that failure - I guess the > > only solution there is not to use FD_ISSET or to cap at max-fds rather than > > the arbitrary 1024*1024 ? > > There can be legitimate programs which use select() in excess of glibc-imposed > artificial limit on bit array sizes. > > (I guess you could argue that's a false positive from fortify, but there > > again I think it is an illegal use of FD_ISSET). > > Do you see a reasonably portable way to check FD_ISSET? Nope; I think as soon as we go beyond sizeof(fd_set) all bets are off and even that seems to be an evil way of telling. > On the related note, how are we doing in "stracing 32-bit app > with 64-bit strace on a big-endian machine" case? > In that case, sizeof(long) is important... > I dread to think about that:( Yeh that's kind of related to the question I asked in my 1st post; since we're getting passed args here as long* and this is actually an int does this work at all on BE machines? Is there GET_ARG_INT thing in strace this routine should be using? Yeh mixed ABI is hard - and things mix ABI in different ways, some systems have to have it in different processes (in which case two strace binaries might do it) and some can mix and match in the one (in which case an strace lib built in the other endianness and called with libffi might work?) Dave -- -----Open up your eyes, open up your mind, open up your code ------- / Dr. David Alan Gilbert | Running GNU/Linux | Happy \ \ gro.gilbert @ treblig.org | | In Hex / \ _________________________|_____ http://www.treblig.org |_______/ |
From: Dmitry V. L. <ld...@al...> - 2013-11-06 00:06:12
|
On Tue, Nov 05, 2013 at 04:19:31PM +0100, Denys Vlasenko wrote: > On 11/05/2013 01:33 PM, Dr. David Alan Gilbert wrote: > > * Denys Vlasenko (dvl...@re...) wrote: > >> I propose to do simply this: > >> > >> + nfds = fdsize; > >> fdsize = (((fdsize + 7) / 8) + sizeof(long)-1) & -sizeof(long); > >> + /* We had bugs a-la "while (j < args[0])" and "umoven(args[0])" below. > >> + * Instead of args[0], use nfds for fd count, fdsize for array lengths. > >> + */ > >> > >> and use nfds in those two places where we incorrectly use arg[0] now. > > > >>> Thoughts? > >> > >> I applied a slightly simplified version of your fix to strace git, please try it. > > > > That still fails (this is FORTIFY detecting the fail). > > Please elaborate. You get a warning about access to fd_set->[__]fds_bits > array past its declared bounds? How it looks? Does strace abort or what? sizeof(fd_set) is part of libc ABI, so glibc in _FORTIFY_SOURCE mode aborts the process using __chk_fail() when descriptor is larger than allowed value (FD_SETSIZE at the time of compiling glibc). > There can be legitimate programs which use select() in excess of glibc-imposed > artificial limit on bit array sizes. Yes, the syscall itself imposes no such limitations. Such programs are more expected to use poll() instead of select(), though. > > (I guess you could argue that's a false positive from fortify, but there > > again I think it is an illegal use of FD_ISSET). > > Do you see a reasonably portable way to check FD_ISSET? Looks like all FD_ISSET implementations just test the n-th bit in the array of long ints. I've pushed a commit with yet another FD_ISSET implementation that hopefully does the same. > On the related note, how are we doing in "stracing 32-bit app > with 64-bit strace on a big-endian machine" case? > In that case, sizeof(long) is important... > I dread to think about that:( We cannot make things worse than they were since the beginning. :) -- ldv |
From: Dr. D. A. G. <da...@tr...> - 2013-11-06 00:24:55
|
* Dmitry V. Levin (ld...@al...) wrote: > On Tue, Nov 05, 2013 at 04:19:31PM +0100, Denys Vlasenko wrote: > > On 11/05/2013 01:33 PM, Dr. David Alan Gilbert wrote: > > > * Denys Vlasenko (dvl...@re...) wrote: > > >> I propose to do simply this: > > >> > > >> + nfds = fdsize; > > >> fdsize = (((fdsize + 7) / 8) + sizeof(long)-1) & -sizeof(long); > > >> + /* We had bugs a-la "while (j < args[0])" and "umoven(args[0])" below. > > >> + * Instead of args[0], use nfds for fd count, fdsize for array lengths. > > >> + */ > > >> > > >> and use nfds in those two places where we incorrectly use arg[0] now. > > > > > >>> Thoughts? > > >> > > >> I applied a slightly simplified version of your fix to strace git, please try it. > > > > > > That still fails (this is FORTIFY detecting the fail). > > > > Please elaborate. You get a warning about access to fd_set->[__]fds_bits > > array past its declared bounds? How it looks? Does strace abort or what? > > sizeof(fd_set) is part of libc ABI, so glibc in _FORTIFY_SOURCE mode > aborts the process using __chk_fail() when descriptor is larger than > allowed value (FD_SETSIZE at the time of compiling glibc). > > > There can be legitimate programs which use select() in excess of glibc-imposed > > artificial limit on bit array sizes. > > Yes, the syscall itself imposes no such limitations. > Such programs are more expected to use poll() instead of select(), though. > > > > (I guess you could argue that's a false positive from fortify, but there > > > again I think it is an illegal use of FD_ISSET). > > > > Do you see a reasonably portable way to check FD_ISSET? > > Looks like all FD_ISSET implementations just test the n-th bit in the > array of long ints. I've pushed a commit with yet another FD_ISSET > implementation that hopefully does the same. That looks like it's done the trick - and I prefer the way your set now just reads and casts argv[0] once. Dave -- -----Open up your eyes, open up your mind, open up your code ------- / Dr. David Alan Gilbert | Running GNU/Linux | Happy \ \ gro.gilbert @ treblig.org | | In Hex / \ _________________________|_____ http://www.treblig.org |_______/ |
From: Denys V. <dvl...@re...> - 2013-11-06 10:35:02
|
On 11/06/2013 01:06 AM, Dmitry V. Levin wrote: > On Tue, Nov 05, 2013 at 04:19:31PM +0100, Denys Vlasenko wrote: >> On 11/05/2013 01:33 PM, Dr. David Alan Gilbert wrote: >> There can be legitimate programs which use select() in excess of glibc-imposed >> artificial limit on bit array sizes. > > Yes, the syscall itself imposes no such limitations. > Such programs are more expected to use poll() instead of select(), though. > >>> (I guess you could argue that's a false positive from fortify, but there >>> again I think it is an illegal use of FD_ISSET). >> >> Do you see a reasonably portable way to check FD_ISSET? > > Looks like all FD_ISSET implementations just test the n-th bit in the > array of long ints. I've pushed a commit with yet another FD_ISSET > implementation that hopefully does the same. static int fd_isset(int d, fd_set *fds) { const int bpl = 8 * sizeof(long); long *s = (long *) fds; return !!(s[d / bpl] & (1L << (d % bpl))); } The function is executed *for each bit*. With gcc -Os at least, this will execute a signed division because gcc must ensure "d / bpl" has a correct value for negative d too - it does not know that we never use negative d. 0: 89 f9 mov %edi,%ecx 2: bf 40 00 00 00 mov $0x40,%edi 7: 89 c8 mov %ecx,%eax 9: 99 cltd a: f7 ff idiv %edi <============ c: 89 d1 mov %edx,%ecx e: 48 63 d0 movslq %eax,%rdx 11: 48 8b 04 d6 mov (%rsi,%rdx,8),%rax 15: 48 d3 f8 sar %cl,%rax 18: 83 e0 01 and $0x1,%eax 1b: c3 retq Making bpl "unsigned": 0: 89 f8 mov %edi,%eax 2: 89 f9 mov %edi,%ecx 4: c1 e8 06 shr $0x6,%eax 7: 83 e1 3f and $0x3f,%ecx a: 89 c0 mov %eax,%eax <==== stupid gcc?? c: 48 8b 04 c6 mov (%rsi,%rax,8),%rax 10: 48 d3 f8 sar %cl,%rax 13: 83 e0 01 and $0x1,%eax 16: c3 retq Inlining fd_isset eliminates call overhead at the cost of only 2 bytes: # size desc.o desc.o.inlined text data bss dec hex filename 5101 0 1024 6125 17ed desc.o 5103 0 1024 6127 17ef desc.o.inlined I committed a change to implement these. |
From: Denys V. <dvl...@re...> - 2013-11-06 10:55:14
|
On 11/05/2013 07:27 PM, Dr. David Alan Gilbert wrote: > * Denys Vlasenko (dvl...@re...) wrote: >> On the related note, how are we doing in "stracing 32-bit app >> with 64-bit strace on a big-endian machine" case? >> In that case, sizeof(long) is important... >> I dread to think about that:( > > Yeh that's kind of related to the question I asked in my 1st post; > since we're getting passed args here as long* and this is actually > an int does this work at all on BE machines? > Is there GET_ARG_INT thing in strace this routine should be using? You misunderstood me. The question is not about arg being longs or ints - that is properly covered (or else EVERY syscall would be horribly broken). I'm talking about the long[] arrays pointed by arg[1,2,3]. In 32-bit process, those arrays have 32-bit "longs". If strace is 64-bit, strace's "longs" are 64-bit. Finding an Nth bit via long_array[fd / BITS_PER_LONG] & (1UL << (fd % BITS_PER_LONG)) works if we have a correct idea about sizeof(long), and it also works correctly on little endian even if we don't: fd = 32 if long is 32-bit: long_array[1] & (1 << 0) -- checks bit 0 in 4th byte if long is 64-bit: long_array[0] & (1 << 32) -- checks bit 0 in 4th byte But on big-endian we have a disaster fd = 32 if long is 32-bit: long_array[1] & (1 << 0) -- checks bit 0 in 7th byte if long is 64-bit: long_array[0] & (1 << 32) -- checks bit 0 in 3rd byte NOT THE SAME! > Yeh mixed ABI is hard - and things mix ABI in different ways, > some systems have to have it in different processes (in which > case two strace binaries might do it) This won't work if you want to strace across execve which execs a different-bitness process. |
From: Dr. D. A. G. <da...@tr...> - 2013-11-06 13:56:53
|
* Denys Vlasenko (dvl...@re...) wrote: > On 11/05/2013 07:27 PM, Dr. David Alan Gilbert wrote: > > * Denys Vlasenko (dvl...@re...) wrote: > >> On the related note, how are we doing in "stracing 32-bit app > >> with 64-bit strace on a big-endian machine" case? > >> In that case, sizeof(long) is important... > >> I dread to think about that:( > > > > Yeh that's kind of related to the question I asked in my 1st post; > > since we're getting passed args here as long* and this is actually > > an int does this work at all on BE machines? > > Is there GET_ARG_INT thing in strace this routine should be using? > > You misunderstood me. The question is not about arg being longs > or ints - that is properly covered (or else EVERY syscall would be > horribly broken). > > I'm talking about the long[] arrays pointed by arg[1,2,3]. > In 32-bit process, those arrays have 32-bit "longs". > If strace is 64-bit, strace's "longs" are 64-bit. > > Finding an Nth bit via > > long_array[fd / BITS_PER_LONG] & (1UL << (fd % BITS_PER_LONG)) > > works if we have a correct idea about sizeof(long), > and it also works correctly on little endian > even if we don't: > > fd = 32 > if long is 32-bit: > long_array[1] & (1 << 0) -- checks bit 0 in 4th byte > if long is 64-bit: > long_array[0] & (1 << 32) -- checks bit 0 in 4th byte > > But on big-endian we have a disaster > > fd = 32 > if long is 32-bit: > long_array[1] & (1 << 0) -- checks bit 0 in 7th byte > if long is 64-bit: > long_array[0] & (1 << 32) -- checks bit 0 in 3rd byte > > NOT THE SAME! Yeh ok, but now we're indexing into the array our selves we could just treat it as a series of bytes and pass in flags for endianness/long size and work it out the hard way. Dave -- -----Open up your eyes, open up your mind, open up your code ------- / Dr. David Alan Gilbert | Running GNU/Linux | Happy \ \ gro.gilbert @ treblig.org | | In Hex / \ _________________________|_____ http://www.treblig.org |_______/ |
From: Denys V. <dvl...@re...> - 2013-11-08 11:27:32
|
On 11/06/2013 01:06 AM, Dmitry V. Levin wrote: > Looks like all FD_ISSET implementations just test the n-th bit in the > array of long ints. I've pushed a commit with yet another FD_ISSET > implementation that hopefully does the same. > >> On the related note, how are we doing in "stracing 32-bit app >> with 64-bit strace on a big-endian machine" case? >> In that case, sizeof(long) is important... >> I dread to think about that:( > > We cannot make things worse than they were since the beginning. :) Testing each bit with FD_ISSET is silly in any case, so I attempted to fix both "bit-endian wordsize bug" and performance problem in one go, by introducing a next_set_bit() function which just gives us a next bit's position. It skips over entire zero bytes. By using bytes instead of words, it can walk bytes in big-endian sequence suitable for current_wordsize without the need of awkward code to access a runtime-variable-width word. It also skips zero bytes even in not-fully zeroed words, which may be a win on speed. Run-tested, although I do not have the environment to reproduce the bit-endian wordsize bug. Dmitry, please review. Patch #2 just syncs up select decoding in pathtrace.c with the main one. -- vda |
From: Dmitry V. L. <ld...@al...> - 2013-11-11 14:43:41
|
On Fri, Nov 08, 2013 at 12:27:18PM +0100, Denys Vlasenko wrote: > On 11/06/2013 01:06 AM, Dmitry V. Levin wrote: > > Looks like all FD_ISSET implementations just test the n-th bit in the > > array of long ints. I've pushed a commit with yet another FD_ISSET > > implementation that hopefully does the same. > > > >> On the related note, how are we doing in "stracing 32-bit app > >> with 64-bit strace on a big-endian machine" case? > >> In that case, sizeof(long) is important... > >> I dread to think about that:( > > > > We cannot make things worse than they were since the beginning. :) > > Testing each bit with FD_ISSET is silly in any case, so I attempted to fix > both "bit-endian wordsize bug" and performance problem in one go, > by introducing a next_set_bit() function which just gives us > a next bit's position. > > It skips over entire zero bytes. By using bytes instead of words, > it can walk bytes in big-endian sequence suitable for current_wordsize > without the need of awkward code to access a runtime-variable-width word. > > It also skips zero bytes even in not-fully zeroed words, > which may be a win on speed. > > Run-tested, although I do not have the environment to reproduce > the bit-endian wordsize bug. > > Dmitry, please review. > Patch #2 just syncs up select decoding in pathtrace.c with the main one. Nice fix, thanks. There is a typo in comment, though. :) -- ldv |