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

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

[bugzilla-daemon]

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

           Summary: Array index out of bounds in tftp_server.c
           Product: eCos
           Version: CVS
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: minor
          Priority: low
         Component: TCP/IP
        AssignedTo: unassigned@bugs.ecos.sourceware.org
        ReportedBy: grant.b.edwards@gmail.com
                CC: ecos-bugs@ecos.sourceware.org
             Class: Advice Request


There appears to be an array index out-of-bounds problem in
tftp_server.c at line 691.  At lines 661-666 there is a for loop that
leaves i with the value CYGNUM_NET_MAX_INET_PROTOS.  Then at line 691,
'i' is used as a subscript in the experess 'server->s[i]'.  The array
's' contains CYGNUM_NET_MAX_INET_PROTOS elements, so the max legal
subscript is CYGNUM_NET_MAX_INET_PROTOS-1, but i is
CYGNUM_NET_MAX_INET_PROTOS at that point.

I have no clue what's going on in this particular code.  The use of i
as a loop index inside an outer loop that also uses i as the loop
index seems like a mistake.  I suspect that the loop at lines 661-666
should not be using i as the loop index.


   647          for (i=0; i < CYGNUM_NET_MAX_INET_PROTOS; i++) {
   648            if (server->s[i] && FD_ISSET(server->s[i],&readfds)) {
   649              recv_len = sizeof(data);
   650              from_len = sizeof(from_addr);
   651              data_len = recvfrom(server->s[i], hdr, recv_len, 0,
   652                                  &from_addr, &from_len);
   653              if ( data_len < 0) {
   654                diag_printf("TFTPD [%x]: can't read request\n", p);
   655              } else {
   656  #ifdef CYGSEM_NET_TFTPD_MULTITHREADED
   657                // Close the socket and post on the semaphore some
   658                // another thread can start listening for requests. This
   659                // is not quite right. select could of returned with more
than
   660                // one socket with data to read. Here we only deal with
one of them
   661                for (i=0; i < CYGNUM_NET_MAX_INET_PROTOS; i++) {
   662                  if (server->s[i]) {
   663                    close (server->s[i]);
   664                    server->s[i] = 0;
   665                  }
   666                }
   667                sem_post(server->port);
   668  #endif
   669  #ifndef CYGPKG_NET_TESTS_USE_RT_TEST_HARNESS
   670                getnameinfo(&from_addr,sizeof(from_addr), name,
sizeof(name),0,0,0);
   671                diag_printf("TFTPD [%x]: received %x from %s\n", p,
   672                            ntohs(hdr->th_opcode), name);
   673  #endif
   674                switch (ntohs(hdr->th_opcode)) {
   675                case WRQ:
   676                  tftpd_write_file(server, hdr, &from_addr, from_len);
   677                  break;
   678                case RRQ:
   679                  tftpd_read_file(server, hdr, &from_addr, from_len);
   680                  break;
   681                case ACK:
   682                case DATA:
   683                case ERROR:
   684                  // Ignore
   685                  break;
   686                default:
   687                  getnameinfo(&from_addr,sizeof(from_addr), name,
sizeof(name),0,0,0);
   688                  diag_printf("TFTPD [%x]: bogus request %x from %s\n",
p,
   689                              ntohs(hdr->th_opcode),
   690                              name);
   691                 
tftpd_send_error(server->s[i],hdr,TFTP_EBADOP,&from_addr,from_len);
   692                }
   693

It seems clear that the loop at 661-666 should not be using i.

However, I'm reluctant to change it without more discussion since I
don't use tftp and don't know how such a "fix" would change the code's
behavior.  Somebody may be depending somehow on the existing behavior
-- it wouldn't be the first time that fixing a bug broke something.

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

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

[bugzilla-daemon]

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

Jonathan Larmour <jifl@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jifl@ecoscentric.com

--- Comment #1 from Jonathan Larmour <jifl@ecoscentric.com> 2012-03-09 05:09:26 GMT ---
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; // at top of function

              for (i=0; i < CYGNUM_NET_MAX_INET_PROTOS; i++) {
                if (server->s[i]) {
                  close (server->s[i]);
                  server->s[i] = 0;
                }
              }


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.

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

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

[bugzilla-daemon]

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.

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

[bugzilla-daemon]

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

--- Comment #3 from Grant Edwards <grant.b.edwards@gmail.com> 2012-03-09 15:55:07 GMT ---
(In reply to comment #2)

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

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

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

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

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

If that were fixed by creating a seperate index for the "close loop",
then the bug you're describing would occur: sending data out using
descriptor 0 because we just closed the server->s[i] and put a "0"
sentinal value in s[].

Why not move the "close loop" to below the switch statement that calls
tftpd_send_error()?

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

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

[bugzilla-daemon]

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

--- Comment #4 from Grant Edwards <grant.b.edwards@gmail.com> 2012-03-09 16:11:42 GMT ---
(In reply to comment #3)

> 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?

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"?

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?

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

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

[bugzilla-daemon]

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 the assignee for the bug.

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

[bugzilla-daemon]

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.

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

[bugzilla-daemon]

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

--- Comment #7 from Jonathan Larmour <jifl@ecoscentric.com> 2012-03-11 03:56:15 GMT ---
(In reply to comment #6)
> > > 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.

Well, my idea was (theoretically) that you could still open a new socket while
still using the previous one to talk to the remote host. But anyway, we happily
seem to have moved onto getting rid of this closing sockets malarkey.

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

It's a small window, but no less present. That's arguably the worst sort of
failure - stuff that works 99% of the time, but inexplicably not the other 1%.

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

Indeed.

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

I think that's a small price to pay. Most likely the number of threads would be
very small anyway.

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

That's not too bad. Although the code may get messy when still retaining the
non-multi-threaded case. But it this was implemented, it wouldn't be a huge
deal to insist in CDL that the number of threads is an even number if ipv6 is
enabled (in the multi-threaded case i.e. >1 ). Or even have the user set the
number of ipv4 threads and ipv6 threads separately.

I think I would prefer the non-blocking socket and blocking select() option
though as IMO the overall penalty for waking multiple threads unnecessarily
occasionally is much less than the overall penalty of having extra threads
around unused.

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

Indeed. Let's not go that route.

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.

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

[bugzilla-daemon]

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

--- Comment #8 from Grant Edwards <grant.b.edwards@gmail.com> 2012-03-12 14:31:57 GMT ---
(In reply to comment #7)
> (In reply to comment #6)


>>> 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.
> 
> Well, my idea was (theoretically) that you could still open a new
> socket while still using the previous one to talk to the remote
> host.

Ah.  I had assumed that wasn't possible (and that's why the sockets
were being closed in the first place).


>>> 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.
> 
> I think that's a small price to pay. Most likely the number of
> threads would be very small anyway.

True.  Efficient but broken is a bit of false economy.

> > 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.
> 
> That's not too bad. Although the code may get messy when still retaining the
> non-multi-threaded case. But it this was implemented, it wouldn't be a huge
> deal to insist in CDL that the number of threads is an even number if ipv6 is
> enabled (in the multi-threaded case i.e. >1 ). Or even have the user set the
> number of ipv4 threads and ipv6 threads separately.

Right.  I had assumed that the minimal case would be one thread per
socket (protocol) -- the thread code would be the same for all cases
(do a blocking read and handle the received packet).  How the threads
are apportioned amongst the protocols would be an new question.
Letting the user set the number of threads for each protcol would
probably be the simplest.

> I think I would prefer the non-blocking socket and blocking select()
> option though as IMO the overall penalty for waking multiple threads
> unnecessarily occasionally is much less than the overall penalty of
> having extra threads around unused.

Compared to the pool-per-socket idea, that approach doesn't require
any CDL changes and would be more transparent to current users, so it
sounds like the best option.

-- 
Grant

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

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

[bugzilla-daemon]

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

Bernd Edlinger <bernd.edlinger@hotmail.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bernd.edlinger@hotmail.de

--- Comment #9 from Bernd Edlinger <bernd.edlinger@hotmail.de> 2012-08-09 17:05:10 BST ---
Hello,

one other thing with the code is this:
a valid socket handle is a number >= 0.
in line 610, the check is
 if (server->s[server->num_s] < 0 ) {
          diag_printf("TFTPD [%x]: can't open socket\n", p);

but later always:

if (server->s[i]) {
            FD_SET(server->s[i],&readfds);

what if socket returns 0, because we have not stdio, i.e. inside a
daemon?

should'nt it be like this:
if (server->s[i]>=0) {
            FD_SET(server->s[i],&readfds);

and

 if (server->s[i]>=0) {
                  close (server->s[i]);
                  server->s[i] = -1;
                }

???

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

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

[bugzilla-daemon]

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

--- Comment #10 from Grant Edwards <grant.b.edwards@gmail.com> 2012-08-09 17:27:25 BST ---
(In reply to comment #9)

> one other thing with the code is this: a valid socket handle is a
> number >= 0.
>
> if (server->s[i]) {
>             FD_SET(server->s[i],&readfds);
>
> what if socket returns 0, because we have not stdio, i.e. inside a
> daemon?

You're right. There isn't any stdin/stdout/stderr or any concept of a
daemon in eCos, and 0 is a perfectly valid value for a socket file
descriptor.  FWIW: in Unix, daemons do have stdin/stdout/stderr
(descriptors 0,1,2) but they're usually connected to /dev/null.

> should'nt it be like this:
> if (server->s[i]>=0) {
>             FD_SET(server->s[i],&readfds);
> 
> and
> 
>  if (server->s[i]>=0) {
>                   close (server->s[i]);
>                   server->s[i] = -1;
>                 }

Yes, you're correct.

Using 0 as a "none" sentinal value for a socket/file descriptor is a
bug. It's not an unusual mistake to make, and for Unix programs it's
never detected because stdin is (almost) always fd 0, but I tripped
over the same thing in some other eCos code I wrote years ago.

Want to submit a patch?

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

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

[bugzilla-daemon]

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

--- Comment #11 from Bernd Edlinger <bernd.edlinger@hotmail.de> 2012-08-09 18:01:40 BST ---
(In reply to comment #10)
> Yes, you're correct.
> Using 0 as a "none" sentinal value for a socket/file descriptor is a
> bug. It's not an unusual mistake to make, and for Unix programs it's
> never detected because stdin is (almost) always fd 0, but I tripped
> over the same thing in some other eCos code I wrote years ago.
> Want to submit a patch?

Well, ok. But is no one working on the idea with the blocking socket reads?

If I am to fix that issue I would prefer a completely simple solution.

1. let only one thread enter the select.
2. never close the sockets, because that throws any additionally received
packets away.
3. post the semaphore before the switch(ntohs(hdr->th_opcode))
4. wait for the semaphore again, and go directly to the select.


Bernd.

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

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

[bugzilla-daemon]

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

--- Comment #12 from Grant Edwards <grant.b.edwards@gmail.com> 2012-08-09 18:33:46 BST ---
(In reply to comment #11)
> (In reply to comment #10)
> > Yes, you're correct.
> > Using 0 as a "none" sentinal value for a socket/file descriptor is a
> > bug. It's not an unusual mistake to make, and for Unix programs it's
> > never detected because stdin is (almost) always fd 0, but I tripped
> > over the same thing in some other eCos code I wrote years ago.

Ah, it looks we've already hashed through the "0 is a valid socket"
thing in this thread.  Sorry about repeating myself.

> > Want to submit a patch?
> Well, ok. But is no one working on the idea with the blocking socket reads?

I'm afraid I'm not.  I don't use tftp and don't even have any
filesystem which it could access.  I only stumbled across the original
bug because I was getting sick of wasting so much time sorting through
all the compiler warnings trying figure out which ones mattered and
which ones didn't.

More than once I fell victim to a bug in code I wrote, and about which
gcc had warned me, because there were hundreds and hunders of other
mostly benign warnings generated by existing code.

> If I am to fix that issue I would prefer a completely simple solution.
> 
> 1. let only one thread enter the select.
> 2. never close the sockets, because that throws any additionally
>    received packets away.
> 3. post the semaphore before the switch(ntohs(hdr->th_opcode))
> 4. wait for the semaphore again, and go directly to the select.

I think the existing code is rather badly and fundamentally broken.
If I were going to re-write it I'd probably throw out the whole
multi-thread prallel operation business and do a single-threaded, one
transfer at a time version that's as simple to understand and as
obviously correct as possible.

I don't know what the use case is for multiple parallel transfers.  If
I _were_ to use tftp it would be for firmware updates which I'd force
to be a single transfer at a time anyway.

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

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

[bugzilla-daemon]

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

--- Comment #13 from Bernd Edlinger <bernd.edlinger@hotmail.de> 2012-08-10 10:57:17 BST ---
(In reply to comment #12)
> I'm afraid I'm not.  I don't use tftp and don't even have any
> filesystem which it could access.  I only stumbled across the original
> bug because I was getting sick of wasting so much time sorting through
> all the compiler warnings trying figure out which ones mattered and
> which ones didn't.

I do completely agree with you.

> I think the existing code is rather badly and fundamentally broken.
> If I were going to re-write it I'd probably throw out the whole
> multi-thread prallel operation business and do a single-threaded, one
> transfer at a time version that's as simple to understand and as
> obviously correct as possible.
> I don't know what the use case is for multiple parallel transfers.  If
> I _were_ to use tftp it would be for firmware updates which I'd force
> to be a single transfer at a time anyway.

True.

Maybe this multi-threaded feature should be off by default in the cdl file.

It is nearly impossible to fix it, because if every thread opens
its own socket, the tcp stack does not know which one will be ready
to handle the packet.

Therefore I think, it is better to just fix the warning,
by _not_ sending an error packet in the multi threaded operation.

Nevertheless, i spent some effort to fix the error handling.

However I found more subtle glitches in the error handling logic,
than I had expected!

I tested with the tftp_server_test, with and without multi-thread.
Of course I did just send valid tftp packets with the windows tftp
client, so the default case in the switch was never executed.

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

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

[bugzilla-daemon]

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

--- Comment #14 from Bernd Edlinger <bernd.edlinger@hotmail.de> 2012-08-10 11:16:02 BST ---
Created an attachment (id=1885)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1885)
Patch to fix the warning and some error hanling in tftp_server.c

line 603: need to initalize max_s again, we're in a loop.
line 648: need to skip the for loop entirely.
iine 649: use server->num_s, because the while loop in line 607 could open less
than the max. number of sockets.
line 693: dont care to send an error packet here.
line 743: there was some kind of error. close all sockets, and release the port
semaphore.
line 752: continue the outer loop.
line 776: initialize some data, in case the tftp_stop is called before the
thread runs.
line 813: only free the data it it was possible to stop the thread.
open question: how to clean up the port semaphore here?

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

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

[bugzilla-daemon]

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

           Summary: Array index out of bounds in tftp_server.c
           Product: eCos
           Version: CVS
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: minor
          Priority: low
         Component: TCP/IP
        AssignedTo: unassigned@bugs.ecos.sourceware.org
        ReportedBy: grant.b.edwards@gmail.com
                CC: ecos-bugs@ecos.sourceware.org
             Class: Advice Request


There appears to be an array index out-of-bounds problem in
tftp_server.c at line 691.  At lines 661-666 there is a for loop that
leaves i with the value CYGNUM_NET_MAX_INET_PROTOS.  Then at line 691,
'i' is used as a subscript in the experess 'server->s[i]'.  The array
's' contains CYGNUM_NET_MAX_INET_PROTOS elements, so the max legal
subscript is CYGNUM_NET_MAX_INET_PROTOS-1, but i is
CYGNUM_NET_MAX_INET_PROTOS at that point.

I have no clue what's going on in this particular code.  The use of i
as a loop index inside an outer loop that also uses i as the loop
index seems like a mistake.  I suspect that the loop at lines 661-666
should not be using i as the loop index.


   647          for (i=0; i < CYGNUM_NET_MAX_INET_PROTOS; i++) {
   648            if (server->s[i] && FD_ISSET(server->s[i],&readfds)) {
   649              recv_len = sizeof(data);
   650              from_len = sizeof(from_addr);
   651              data_len = recvfrom(server->s[i], hdr, recv_len, 0,
   652                                  &from_addr, &from_len);
   653              if ( data_len < 0) {
   654                diag_printf("TFTPD [%x]: can't read request\n", p);
   655              } else {
   656  #ifdef CYGSEM_NET_TFTPD_MULTITHREADED
   657                // Close the socket and post on the semaphore some
   658                // another thread can start listening for requests. This
   659                // is not quite right. select could of returned with more
than
   660                // one socket with data to read. Here we only deal with
one of them
   661                for (i=0; i < CYGNUM_NET_MAX_INET_PROTOS; i++) {
   662                  if (server->s[i]) {
   663                    close (server->s[i]);
   664                    server->s[i] = 0;
   665                  }
   666                }
   667                sem_post(server->port);
   668  #endif
   669  #ifndef CYGPKG_NET_TESTS_USE_RT_TEST_HARNESS
   670                getnameinfo(&from_addr,sizeof(from_addr), name,
sizeof(name),0,0,0);
   671                diag_printf("TFTPD [%x]: received %x from %s\n", p,
   672                            ntohs(hdr->th_opcode), name);
   673  #endif
   674                switch (ntohs(hdr->th_opcode)) {
   675                case WRQ:
   676                  tftpd_write_file(server, hdr, &from_addr, from_len);
   677                  break;
   678                case RRQ:
   679                  tftpd_read_file(server, hdr, &from_addr, from_len);
   680                  break;
   681                case ACK:
   682                case DATA:
   683                case ERROR:
   684                  // Ignore
   685                  break;
   686                default:
   687                  getnameinfo(&from_addr,sizeof(from_addr), name,
sizeof(name),0,0,0);
   688                  diag_printf("TFTPD [%x]: bogus request %x from %s\n",
p,
   689                              ntohs(hdr->th_opcode),
   690                              name);
   691                 
tftpd_send_error(server->s[i],hdr,TFTP_EBADOP,&from_addr,from_len);
   692                }
   693

It seems clear that the loop at 661-666 should not be using i.

However, I'm reluctant to change it without more discussion since I
don't use tftp and don't know how such a "fix" would change the code's
behavior.  Somebody may be depending somehow on the existing behavior
-- it wouldn't be the first time that fixing a bug broke something.

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