[Bug 1001522] Array index out of bounds in tftp_server.c

[bugzilla-daemon@bugs.ecos.sourceware.org]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001522

--- Comment #5 from Jonathan Larmour <jifl@ecoscentric.com> 2012-03-09 16:32:07 GMT ---
(In reply to comment #3)
> (In reply to comment #2)
> > However, if the client sends an unsupported opcode, then this bit runs:
> >   tftpd_send_error(server->s[i],hdr,TFTP_EBADOP,&from_addr,from_len);
> > the problem here being that we've just closed server->s[i].
> 
> But I don't think we did just close server->s[i].
> 
> At that point, i == CYGNUM_NET_MAX_INET_PROTOS
> 
> We closed server->s[0] through server->s[CYGNUM_NET_MAX_INET_PROTOS-1]
> 
> server->s[i] is past the end of array server->s[].

True. So it's doubly broken :-).

> > So that's broken.  Either it should make a new socket and use that
> > for sending, or send the error before it closes this server
> > socket. A cheaty solution is probably something like:
> >
> >               int server_sock = -1; // at top of function
> >               // ...
> >               for (i=0; i < CYGNUM_NET_MAX_INET_PROTOS; i++) {
> >                 if (server->s[i]) {
> >                   server_sock = server->s[i];
> >                   server->s[i] = 0;
> >                 }
> >               }
> > 
> > and then use server_sock later on, and close it after the end of the 'switch'
> > block.
> 
> Wouldn't that leak sockets if more than one is open?  The loop would
> set all of them to 0 [which is also a bug, see below], but it only
> remembers the last one for closing later.

Oh true. Yes you have to close them all _except_ the one that matched select.

> Using the integer value 0 as a sentinal value meaning "no socket" is a
> problem.  If the tftp server is the first task to create a socket,
> it's going to get fd 0.  Using -1 to mean "no socket" is fine.  [I've
> been burned by the 0 == no-socket bug a couple times times in the past
> few years in our eCos application code.]

Yep, that's true too.

> > It's all rather crufty code - there's a window where the server socket is
> > closed and hasn't been reopened (which will cause ICMP port unreachable
> > messages to be sent, not merely have the TFTP request be ignored). It shouldn't
> > be closed at all really, but that would require other changes too. And I
> > dislike the server handle being a cast to an int of the server address. The
> > whole setup just isn't very good.
> > 
> > What do you think?
> 
> I still think the "close loop" shouldn't be using i as an index
> variable since it's inside an outer loop that's using i as an index
> variable.  That's causing the call to tftpd_send_error() to access
> past the end of access server->s[].

Yes I see it will be easier to sort this out with a different variable.

>> Why not move the "close loop" to below the switch statement that calls
>> tftpd_send_error()?
>
> Because then the sockets don't get closed until after the file
> operation completes and you loose the possibility of doing multiple
> file operations in parallel?

Exactly. With the current structure anyway.

> If that's the case, then the ckeck for invalid opcode and error
> response needs to happen before the "close loop", but the actual
> handling of the read/write opcode needs to happen after the "close
> loop"?

Or we keep the socket around. Or we make a new socket to use just to send the
error.

> I must admit, I don't really understand why the sockets are being
> closed at all.  Can't multiple threads read from a single socket?

Indeed, hence my earlier comment about the crufty code. Of course if all
threads run select to wait for socket events, then they'll all wake up when any
event comes in, which therefore requires further thread synchronisation to sort
out. At a guess, that's the reason for the current "short-cuts".

A proper solution would involve a master thread, and a number of child threads.

But maybe a good enough solution which isn't too far from current is to make
the socket non-blocking, and just get threads to loop back to the select if
they wake up and there's no data after all (recvfrom() returns -1 with errno
set to EWOULDBLOCK).

Jifl

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.