public inbox for ecos-bugs@sourceware.org help / color / mirror / Atom feed
From: bugzilla-daemon@bugs.ecos.sourceware.org To: unassigned@bugs.ecos.sourceware.org Subject: [Bug 1001522] Array index out of bounds in tftp_server.c Date: Fri, 09 Mar 2012 17:39:00 -0000 [thread overview] Message-ID: <20120309173854.506702F78001@mail.ecoscentric.com> (raw) In-Reply-To: <bug-1001522-777@http.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 #6 from Grant Edwards <grant.b.edwards@gmail.com> 2012-03-09 17:38:51 GMT --- (In reply to comment #5) > > 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 :-). oh, at least doubly... > > > 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. Then you can have parallel operations only when requests are coming in simultaneously on different protocols. > > 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. The whole thing seems to be coming rather badly unravelled... > > > 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). Indeed, I hadn't thought about that. > > > 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. OK, we'll take it as a given that it will use a different index variable for the close loop. I think the sentinel problem needs to be fixed as well. (I mean the use of 0 -- not my repeated misspelling of the word "sentinel".) There's still the issue of how to send an error response, and the problem with the port-unreachable window. >>> 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. Yep -- in which case you lose parallelism completely in the case where only one protocol (e.g. ipv4) is being used. > Or we make a new socket to use just to send the error. That would work, and the overhead is only there when the error condition happens. It seems to me it would be simpler to check the opcode and send the error resonse before closing the sockets. It's simple and keeps the parallelism, but it doesn't fix the port-unreachable problem (if it is indeed a problem in the real world). >> 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. That's the first solution that occured to me, but it's a lot of rework. I don't use the tftp support and don't use filesystem support, so it would be pretty far down my list of things to do (to be honest, it would probably never make it to the top). > 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). That's probably the easiest way to fix the port-unreachable problem, but it does have the overhead of waking all the threads for every packet. Another solution that occurred to me is to have a set of threads for each socket (IOW for each protocol). Leave the sockets blocking and keep them open the whole time. All the threads make blocking read calls, and only one wakes up for each packet. It's all nice and simple, but you need more thread instances _if_ ipv6 is enabled. Having a single thread doing the select()/read() and passing the incoming packets to a pool of worker threads would be a bit more elegent, but it's also a bit more complicated, and you pay that extra overhead even when you only have one protocol enabled. -- Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the assignee for the bug.
next prev parent reply other threads:[~2012-03-09 17:39 UTC|newest] Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top 2012-03-07 15:43 [Bug 1001522] New: " bugzilla-daemon 2012-03-09 5:09 ` [Bug 1001522] " bugzilla-daemon 2012-03-09 5:13 ` bugzilla-daemon 2012-03-09 15:55 ` bugzilla-daemon 2012-03-09 16:12 ` bugzilla-daemon 2012-03-09 16:32 ` bugzilla-daemon 2012-03-09 17:39 ` bugzilla-daemon [this message] 2012-03-11 3:56 ` bugzilla-daemon 2012-03-12 14:32 ` bugzilla-daemon 2012-08-09 16:05 ` bugzilla-daemon 2012-08-09 16:27 ` bugzilla-daemon 2012-08-09 17:02 ` bugzilla-daemon 2012-08-09 17:34 ` bugzilla-daemon 2012-08-10 9:57 ` bugzilla-daemon 2012-08-10 10:16 ` bugzilla-daemon -- strict thread matches above, loose matches on Subject: below -- 2012-03-07 15:43 [Bug 1001522] New: " bugzilla-daemon 2012-03-09 5:09 ` [Bug 1001522] " bugzilla-daemon 2012-03-09 5:13 ` bugzilla-daemon 2012-03-09 15:55 ` bugzilla-daemon 2012-03-09 16:12 ` bugzilla-daemon 2012-03-09 16:32 ` bugzilla-daemon 2012-03-09 17:39 ` bugzilla-daemon 2012-03-11 3:56 ` bugzilla-daemon 2012-03-12 14:32 ` bugzilla-daemon 2012-08-09 16:05 ` bugzilla-daemon 2012-08-09 16:28 ` bugzilla-daemon 2012-08-09 17:02 ` bugzilla-daemon 2012-08-09 17:34 ` bugzilla-daemon 2012-08-10 9:57 ` bugzilla-daemon 2012-08-10 10:16 ` bugzilla-daemon
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20120309173854.506702F78001@mail.ecoscentric.com \ --to=bugzilla-daemon@bugs.ecos.sourceware.org \ --cc=unassigned@bugs.ecos.sourceware.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).