>Number:         32680
>Category:       kern
>Synopsis:       Missing error handling in tftp, which may cause memory errors
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Jan 31 16:45:01 +0000 2006
>Originator:     Yves-Emmanuel JUTARD
>Release:        3.0.0
>Organization:
THALES Communication
>Environment:
custom environment : recompiled from /src, only some parts of NetBSD are used (TCP/IP stack and some parts of the kernel)
>Description:
In the "tftp" utility, file usr.bin/tftp/tftp.c (v 1.20)
- l. 316 : call to 'fdopen' is not checked for failure
- l. 348 : call to 'write_behind' is not checked for failure
- l. 427 : call to 'write_behind' is not checked, BUT SHOULD NOT, because last write can be of size 0 and thus return an error, but it's not an error. (I had problem with that when trying to fix ;-)

In file usr.bin/tftp/tftpsubs.c (v 1.8)
- l. 193 : call to 'write_behind' is not checked for failure
- l. 245 : call to 'putc' is not checked for failure

Those fixes are not only for the sake of checking errors. On system with limited memory (like mine), those missings checks have caused massive memory corruption when transferring a large file with not enough memory to store it. (Note that I have a custom file management for tftp file access, so in 'official' NetBSD, the effects could be different)

I express once again that the proposed fix works FOR ME, but since I'm not a NetBSD expert, I may have missed some things. So check carefully please. And don't forget to properly retabulate tftp.c. I've not done it for diff clarity.

It's unrelated, but I've also fixed some bad type uses in tftp.c and tftpsubs.c, causing warnings with my compiler.


>How-To-Repeat:
try to download a big file when you don't have any more RAM (and no swap)
>Fix:
--- tftp.c      Sun Oct 10 23:15:34 2004
+++ tftp_corrected.c    Tue Jan 31 09:54:22 2006
@@ -304,7 +304,8 @@
        volatile int size, firsttrip;
        volatile unsigned long amount;
        struct sockaddr_storage from;
-       int fromlen, readlen;
+       socklen_t fromlen;
+       size_t readlen;
        FILE *file;
        volatile int convert;           /* true if converting crlf -> lf */
        struct sockaddr_storage peer;
@@ -314,6 +315,8 @@
        dp = w_init();
        ap = (struct tftphdr *)ackbuf;
        file = fdopen(fd, "w");
+       if (file != NULL)
+       {
        convert = !strcmp(mode, "netascii");
        block = 1;
        firsttrip = 1;
@@ -345,7 +348,10 @@
                        warn("sendto");
                        goto abort;
                }
-               write_behind(file, convert);
+               if (write_behind(file, convert) < 0)
+               {
+                       goto abort;
+               }
                for ( ; ; ) {
                        alarm(rexmtval);
                        do  {
@@ -424,8 +430,14 @@
        ap->th_block = htons((u_short)block);
        (void) sendto(f, ackbuf, 4, 0, (struct sockaddr *)&peer,
            peer.ss_len);
-       write_behind(file, convert);            /* flush last buffer */
+       /* flush last buffer
+        * We do not check for failure because last buffer
+        * can be empty, thus returning an error.
+        * XXX maybe we should fix 'write_behind' instead.
+        */
+       write_behind(file, convert);
        fclose(file);
+       }
        stopclock();
        if (amount > 0)
                printstats("Received", amount);



--- tftpsubs.c  Thu Aug  7 12:16:14 2003
+++ tftpsubs_corrected.c        Tue Jan 31 10:03:13 2006
@@ -190,7 +190,12 @@
        bfs[current].counter = ct;      /* set size of data to write */
        current = !current;             /* switch to other buffer */
        if (bfs[current].counter != BF_FREE)     /* if not free */
-               (void)write_behind(file, convert); /* flush it */
+       {
+               if (write_behind(file, convert) < 0) /* flush it */
+               {
+                       return -1; /* error... */
+               }
+       }
        bfs[current].counter = BF_ALLOC;        /* mark as alloc'd */
        *dpp =  (struct tftphdr *)bfs[current].buf;
        return ct;                      /* this is a lie of course */
@@ -242,7 +247,10 @@
                        goto skipit;    /* just skip over the putc */
                /* else just fall through and allow it */
            }
-           putc(c, file);
+           if (putc(c, file) == EOF)
+           {
+               return -1; /* error... */
+           }
 skipit:
            prevchar = c;
        }
@@ -269,7 +277,7 @@
        int i, j = 0;
        char rbuf[PKTSIZE];
        struct sockaddr_storage from;
-       int fromlen;
+       socklen_t fromlen;

        while (1) {
                (void) ioctl(f, FIONREAD, &i);