Mon 13 Mar 2017 08:04:49 PM UTC, original submission:
I am currently developing a TCP and UDP application on NXP Kinetis using lwIP 2.0.0. NXP does not provide any of the apps, so I copied lwip/src/apps/tftp/tftp_server.c into my application. Everything seemed to work OK, except after some extended testing, I noticed I was sometimes getting trailing garbage in my filenames:
I ran Wireshark and verified that my TFTP client (Microsoft command line) was not generating bad filenames. I traced into my application where I was generating the above messages. I then went down the stack one level into tftp_server.c, static void recv().
Long story short, the math for calculating the size of the filename data to copy, which must include the terminating null, is off by one. I'll include an example, by a read request for file "wibble", mode "octet".
A TFTP read request consists of a 2-byte opcode, followed by a null-terminated filename, followed by a null-terminated mode. The opcode always starts the filename at offset 2. So in this call:
filename_end_offset = pbuf_memfind(p, &tftp_null, sizeof(tftp_null), 2);
filename_end_offset will for the above example be set to 8 (opcode is 2 bytes, "wibble" is six bytes long). The code then uses the expression (filename_end_offset-2) in two places: checking it does not exceed sizeof(filename), and setting the size of data to copy to filename[]. For the above example, the expression equals 6. But that's not the value you want... you need to copy the trailing null as well.
You actually want ((filename_end_offset-2)+1), or (filename_end_offset-1), which in this example is 7. You want to change the expression in both places; you need to make sure that the string PLUS null does not exceed the filename[] buffer, and you need to copy the string PLUS null to the buffer.
Now, I also checked the code for the mode string, and that is correct; filename_end_offset+1 is indeed the offset of the start of the mode string, and (mode_end_offset-filename_end_offset) evaluates correctly in describing the length of the mode string plus null. Also, the declarations
char filename[TFTP_MAX_FILENAME_LEN];
char mode[TFTP_MAX_MODE_LEN];
are not semantically correct, since those buffers need to contain a null; I suggest adding "+1" to each [] expression. Thus, if TFTP_MAX_FILENAME_LEN is 20, then you can indeed supply a 20-character filename, and filename[TFTP_MAX_FILENAME_LEN+1] can hold 20 characters plus a null.
I've tagged this bug against 2.0.2, because doing a diff shows that tftp_server.c has not changed from 2.0.0 to 2.0.2. I've attached a fixed copy of tftp_server.c that runs correctly on my target. This is my first bug report for this project, so let me know if there's anything else I need to do, thanks.
|