This is the mail archive of the ecos-bugs@sourceware.org mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]