[ECOS] Re: array index bug in tftp_server.c

[Grant Edwards <grant.b.edwards@gmail.com>]

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