* Denys Vlasenko ([email protected]) 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 |_______/
------------------------------------------------------------------------------
November Webinars for C, C++, Fortran Developers
Accelerate application performance with scalable programming models. Explore
techniques for threading, error checking, porting, and tuning. Get the most
from the latest Intel processors and coprocessors. See abstracts and register
http://pubads.g.doubleclick.net/gampad/clk?id=60136231&iu=/4140/ostg.clktrk
_______________________________________________
Strace-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/strace-devel
