* [ECOS] array index bug in tftp_server.c
@ 2012-03-01 23:39 Grant Edwards
2012-03-01 23:45 ` Gary Thomas
0 siblings, 1 reply; 3+ messages in thread
From: Grant Edwards @ 2012-03-01 23:39 UTC (permalink / raw)
To: ecos-discuss
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
--
Grant Edwards grant.b.edwards Yow! I hope I bought the
at right relish ... zzzzzzzzz
gmail.com ...
--
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [ECOS] array index bug in tftp_server.c
2012-03-01 23:39 [ECOS] array index bug in tftp_server.c Grant Edwards
@ 2012-03-01 23:45 ` Gary Thomas
2012-03-02 0:27 ` [ECOS] " Grant Edwards
0 siblings, 1 reply; 3+ messages in thread
From: Gary Thomas @ 2012-03-01 23:45 UTC (permalink / raw)
To: ecos-discuss
On 2012-03-01 16:38, Grant Edwards wrote:
> 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 looks like the loop variable 'i' is reused (incorrectly)
inside the #ifdef CYGSEM_NET_TFTPD_MULTITHREADED starting
on line 656. Change that to be a different variable and it
should fix it.
--
------------------------------------------------------------
Gary Thomas | Consulting for the
MLB Associates | Embedded world
------------------------------------------------------------
--
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss
^ permalink raw reply [flat|nested] 3+ messages in thread* [ECOS] Re: array index bug in tftp_server.c
2012-03-01 23:45 ` Gary Thomas
@ 2012-03-02 0:27 ` Grant Edwards
0 siblings, 0 replies; 3+ messages in thread
From: Grant Edwards @ 2012-03-02 0:27 UTC (permalink / raw)
To: ecos-discuss
On 2012-03-01, Gary Thomas <gary@mlbassoc.com> wrote:
> On 2012-03-01 16:38, Grant Edwards wrote:
>> 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 looks like the loop variable 'i' is reused (incorrectly)
> inside the #ifdef CYGSEM_NET_TFTPD_MULTITHREADED starting
> on line 656. Change that to be a different variable and it
> should fix it.
I agree. I'd be happy to submit a patch, but I don't actually use the
tftp support (as of a few minutes ago I no longer even build it). I
could re-enable it and do a build to make sure the compiler warning
goes away, but I don't really have anything setup for testing it. It
does look like there is some sort of autotest that includes tftp...
--
Grant Edwards grant.b.edwards Yow! On the road, ZIPPY
at is a pinhead without a
gmail.com purpose, but never without
a POINT.
--
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-03-02 0:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-01 23:39 [ECOS] array index bug in tftp_server.c Grant Edwards
2012-03-01 23:45 ` Gary Thomas
2012-03-02 0:27 ` [ECOS] " Grant Edwards
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).