diff options
| author | Dr. David Alan Gilbert <dave@treblig.org> | 2013-11-05 11:54:51 +0100 |
|---|---|---|
| committer | Denys Vlasenko <dvlasenk@redhat.com> | 2013-11-05 11:54:51 +0100 |
| commit | 025f1082b6c9573772472cc9039c2e10225c2c42 (patch) | |
| tree | ab275532c13c67dcc7d430be1c9810820b5a64f2 | |
| parent | 0b4060f61f1bb101b5d8d084714b7d2feacdb199 (diff) | |
| download | strace-025f1082b6c9573772472cc9039c2e10225c2c42.tar.gz strace-025f1082b6c9573772472cc9039c2e10225c2c42.tar.bz2 strace-025f1082b6c9573772472cc9039c2e10225c2c42.tar.xz | |
Fix select decoding with bogus (huge or negative) nfds.
We used to allocate and fetch bit arrays using a sanitized
length, but then iterate over them with "j < arg[0]" condition,
where arg[0] is not sanitized. This segfaults if arg[0] is huge
or negative. This change fixes this.
Add test/select.c to capture the case.
Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
| -rw-r--r-- | desc.c | 18 | ||||
| -rw-r--r-- | test/.gitignore | 1 | ||||
| -rw-r--r-- | test/Makefile | 2 | ||||
| -rw-r--r-- | test/select.c | 23 |
4 files changed, 36 insertions, 8 deletions
@@ -492,14 +492,17 @@ decode_select(struct tcb *tcp, long *args, enum bitness_t bitness) fdsize = 1024*1024; if (args[0] < 0) fdsize = 0; + 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. + */ if (entering(tcp)) { fds = malloc(fdsize); if (!fds) die_out_of_memory(); - nfds = args[0]; - tprintf("%d", nfds); + tprintf("%ld", args[0]); for (i = 0; i < 3; i++) { arg = args[i+1]; if (arg == 0) { @@ -533,12 +536,13 @@ decode_select(struct tcb *tcp, long *args, enum bitness_t bitness) char *outptr; #define end_outstr (outstr + sizeof(outstr)) const char *sep; + long ready_fds; if (syserror(tcp)) return 0; - nfds = tcp->u_rval; - if (nfds == 0) { + ready_fds = tcp->u_rval; + if (ready_fds == 0) { tcp->auxstr = "Timeout"; return RVAL_STR; } @@ -555,7 +559,7 @@ decode_select(struct tcb *tcp, long *args, enum bitness_t bitness) arg = args[i+1]; if (!arg || umoven(tcp, arg, fdsize, (char *) fds) < 0) continue; - for (j = 0; j < args[0]; j++) { + for (j = 0; j < nfds; j++) { if (FD_ISSET(j, fds)) { /* +2 chars needed at the end: ']',NUL */ if (outptr < end_outstr - (sizeof(", except [") + sizeof(int)*3 + 2)) { @@ -572,12 +576,12 @@ decode_select(struct tcb *tcp, long *args, enum bitness_t bitness) outptr += sprintf(outptr, " %u", j); } } - nfds--; + ready_fds--; } } if (outptr != outstr) *outptr++ = ']'; - if (nfds == 0) + if (ready_fds == 0) break; } free(fds); 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..523d75c --- /dev/null +++ b/test/select.c @@ -0,0 +1,23 @@ +/* dave@treblig.org */ +#include <sys/select.h> +#include <sys/time.h> +#include <sys/types.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> + +char buffer[1024*1024*2]; + +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); + return 0; +} |