[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 #2 from Jonathan Larmour <jifl@ecoscentric.com> 2012-03-09 05:12:51 GMT ---
[ Sigh, I pressed tab too many times and accidentally prematurely submitted
this comment before it was finished. Ignore the previous comment. ]

Yes there's a bug here, but it's not quite that. That inner loop _should_ go
and close both the ipv4 and ipv6 sockets, because once another thread wakes up
to be the tftp server, it will want to open new ones of each. We'd leak sockets
if we didn't close both. So that bit's ok.

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]. 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.

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?

Jifl

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