[SCM] strace branch, master, updated. v4.6-129-g1d46ba5
Denys Vlasenko
dvlasenk at redhat.com
Thu Sep 1 10:51:43 UTC 2011
On Thu, 2011-09-01 at 00:46 +0400, Dmitry V. Levin wrote:
> On Wed, Aug 31, 2011 at 12:01:06PM +0000, Denys Vlasenko wrote:
> > commit 1d46ba57a8ab16b353b531f2bbefe2ad7f354ca9
> > Author: Denys Vlasenko <dvlasenk at redhat.com>
> > Date: Wed Aug 31 14:00:02 2011 +0200
> >
> > Make out-of-memory handling more uniform
> >
> > This fixes one real bug in dumpstr().
> >
> > * defs.h: Declare die_out_of_memory().
> > * strace.c (die_out_of_memory): New function.
> > (strace_popen): If allocation fails, call die_out_of_memory().
> > (main): Likewise.
> > (expand_tcbtab): Likewise.
> > (rebuild_pollv): Likewise.
> > * count.c (count_syscall): Likewise.
> > (call_summary_pers): Likewise.
> > * desc.c (decode_select): Likewise.
> > * file.c (sys_getdents): Likewise.
> > (sys_getdents64): Likewise.
> > (sys_getdirentries): Likewise.
> > * pathtrace.c (pathtrace_match): Likewise.
> > * syscall.c (qualify): Likewise.
> > * util.c (printstr): Likewise.
> > (dumpiov): Likewise.
> > (dumpstr): Likewise.
> > (fixvfork): Likewise.
> > * mem.c (sys_mincore): Don't check free() parameter for NULL.
>
> There are no need to hurry with changes like this.
> Not every ENOMEM is really fatal.
>
> > --- a/desc.c
> > +++ b/desc.c
> > @@ -497,9 +497,9 @@ decode_select(struct tcb *tcp, long *args, enum bitness_t bitness)
> > long arg;
> >
> > if (entering(tcp)) {
> > - fds = (fd_set *) malloc(fdsize);
> > - if (fds == NULL)
> > - fprintf(stderr, "out of memory\n");
> > + fds = malloc(fdsize);
> > + if (!fds)
> > + die_out_of_memory();
>
> For example, this memory allocation error is certainly not fatal.
> fdsize value is based on untrusted input and therefore can be quite large.
>
> For example,
>
> $ cat select.c
> #include <sys/select.h>
> int main(void)
> {
> static struct timeval t;
> return select(2147483640, 0, 0, 0, &t) < 0;
> }
>
> It will cause strace to call malloc(268435456), and the old code would
> handle this:
>
> $ (ulimit -v 262144; strace -eselect ./select)
> select(out of memory
> 2147483640, NULL, NULL, NULL, {0, 0}) = 0 (Timeout)
>
> Not as nice as it could be, but better than what the new code does.
I agree, user-supplied sizes should not be trusted.
I propose the following change. It sanitizes fd set sizes,
iov sizes by capping them by a VERY large limits, larger
than any reasonable program would use. And even if the program
is not reasonable, we do not crash, we also do not even try to
allocate gigabytes of RAM: we just don't omit printing of some
information.
In other places, such as dumpstr(), I just revert to old code.
--
vda
diff -d -urpN strace.a/desc.c strace.b/desc.c
--- strace.a/desc.c 2011-08-31 18:58:51.215271680 +0200
+++ strace.b/desc.c 2011-09-01 12:34:25.272438255 +0200
@@ -490,12 +490,19 @@ static int
decode_select(struct tcb *tcp, long *args, enum bitness_t bitness)
{
int i, j, nfds;
- unsigned int fdsize = ((((args[0] + 7) / 8) + sizeof(long) - 1)
- & -sizeof(long));
+ unsigned nfds, fdsize;
fd_set *fds;
const char *sep;
long arg;
+ fdsize = args[0];
+ /* Beware of select(2^31-1, NULL, NULL, NULL) and similar... */
+ if (args[0] > 1024*1024)
+ fdsize = 1024*1024;
+ if (args[0] < 0)
+ fdsize = 0;
+ fdsize = (((fdsize + 7) / 8) + sizeof(long)-1) & -sizeof(long);
+
if (entering(tcp)) {
fds = malloc(fdsize);
if (!fds)
diff -d -urpN strace.a/file.c strace.b/file.c
--- strace.a/file.c 2011-08-31 18:58:51.215271680 +0200
+++ strace.b/file.c 2011-09-01 12:34:41.116590073 +0200
@@ -2421,6 +2421,11 @@ sys_getdents(struct tcb *tcp)
return 0;
}
len = tcp->u_rval;
+ /* Beware of insanely large or negative values in tcp->u_rval */
+ if (tcp->u_rval > 1024*1024)
+ len = 1024*1024;
+ if (tcp->u_rval < 0)
+ len = 0;
buf = len ? malloc(len) : NULL;
if (len && !buf)
die_out_of_memory();
@@ -2502,10 +2507,17 @@ sys_getdents64(struct tcb *tcp)
tprintf("%#lx, %lu", tcp->u_arg[1], tcp->u_arg[2]);
return 0;
}
+
len = tcp->u_rval;
+ /* Beware of insanely large or negative tcp->u_rval */
+ if (tcp->u_rval > 1024*1024)
+ len = 1024*1024;
+ if (tcp->u_rval < 0)
+ len = 0;
buf = len ? malloc(len) : NULL;
if (len && !buf)
die_out_of_memory();
+
if (umoven(tcp, tcp->u_arg[1], len, buf) < 0) {
tprintf("%#lx, %lu", tcp->u_arg[1], tcp->u_arg[2]);
free(buf);
@@ -2573,10 +2585,17 @@ sys_getdirentries(struct tcb *tcp)
tprintf("%#lx, %lu, %#lx", tcp->u_arg[1], tcp->u_arg[2], tcp->u_arg[3]);
return 0;
}
+
len = tcp->u_rval;
+ /* Beware of insanely large or negative tcp->u_rval */
+ if (tcp->u_rval > 1024*1024)
+ len = 1024*1024;
+ if (tcp->u_rval < 0)
+ len = 0;
buf = malloc(len);
if (!buf)
die_out_of_memory();
+
if (umoven(tcp, tcp->u_arg[1], len, buf) < 0) {
tprintf("%#lx, %lu, %#lx", tcp->u_arg[1], tcp->u_arg[2], tcp->u_arg[3]);
free(buf);
diff -d -urpN strace.a/pathtrace.c strace.b/pathtrace.c
--- strace.a/pathtrace.c 2011-08-31 18:58:51.216271686 +0200
+++ strace.b/pathtrace.c 2011-09-01 12:46:32.118719073 +0200
@@ -269,7 +269,8 @@ pathtrace_match(struct tcb *tcp)
s->sys_func == sys_oldselect ||
s->sys_func == sys_pselect6)
{
- int i, j, nfds;
+ int i, j;
+ unsigned nfds;
long *args, oldargs[5];
unsigned fdsize;
fd_set *fds;
@@ -286,10 +287,14 @@ pathtrace_match(struct tcb *tcp)
args = tcp->u_arg;
nfds = args[0];
+ /* Beware of select(2^31-1, NULL, NULL, NULL) and similar... */
+ if (args[0] > 1024*1024)
+ nfds = 1024*1024;
+ if (args[0] < 0)
+ nfds = 0;
fdsize = ((((nfds + 7) / 8) + sizeof(long) - 1)
& -sizeof(long));
fds = malloc(fdsize);
-
if (!fds)
die_out_of_memory();
diff -d -urpN strace.a/util.c strace.b/util.c
--- strace.a/util.c 2011-09-01 11:13:31.919110047 +0200
+++ strace.b/util.c 2011-09-01 12:46:11.109555064 +0200
@@ -686,12 +686,14 @@ dumpiov(struct tcb *tcp, int len, long a
#define iov_iov_len(i) iov[i].iov_len
#endif
int i;
- unsigned long size;
+ unsigned size;
- size = sizeof_iov * (unsigned long) len;
- if (size / sizeof_iov != len /* overflow? */
+ size = sizeof_iov * len;
+ /* Assuming no sane program has millions of iovs */
+ if ((unsigned)len > 1024*1024 /* insane or negative size? */
|| (iov = malloc(size)) == NULL) {
- die_out_of_memory();
+ fprintf(stderr, "Out of memory\n");
+ return;
}
if (umoven(tcp, addr, size, (char *) iov) >= 0) {
for (i = 0; i < len; i++) {
@@ -703,7 +705,7 @@ dumpiov(struct tcb *tcp, int len, long a
iov_iov_len(i));
}
}
- free((char *) iov);
+ free(iov);
#undef sizeof_iov
#undef iov_iov_base
#undef iov_iov_len
@@ -722,8 +724,11 @@ dumpstr(struct tcb *tcp, long addr, int
if (strsize < len) {
free(str);
str = malloc(len);
- if (!str)
- die_out_of_memory();
+ if (!str) {
+ strsize = -1;
+ fprintf(stderr, "Out of memory\n");
+ return;
+ }
strsize = len;
}
More information about the Strace-devel
mailing list