On 2012-03-01, Gary Thomas <g...@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
