[PATCH] Fix 2 possible SEGVs in tftp client
Bernhard Fischer
rep.nop at aon.at
Sat Apr 22 11:13:58 UTC 2006
On Fri, Apr 21, 2006 at 05:04:34PM +0200, KRONSTORFER Horst wrote:
>> On 4/21/06, KRONSTORFER Horst <Horst.KRONSTORFER at frequentis.com> wrote:
>>>
>>> hi!
>>>
>>> attached patch fixes 2 possible segfaults in tftp.c:
>>>
>>> 1) when both -l and -r are omitted, a null pointer is passed for
>>> 'remotefile'
>>> to tftp() which doesn't expect a null pointer for this argument.
>>>
>>> 2) when hostname/ipaddr is omitted, a null pointer is passed for 'name'
>>> to gethostbyname() (via xgethostbyname()) which doesn't expect a null
>>> pointer for this argument.
>>>
>
>> Looks great, I submitted a patch that never got applied for #1 last year,
>> but I hadn't even run into #2. I would suggest whoever applies this also
>> remove the worthless comment:
>
>> /* XXX: argv[optind] and/or argv[optind + 1] may be NULL! */
>
>ack!
>
>-h
>
>> (Gotta love when people comment the bad condition, rather than just check
>> for it).
>
>> Attached is another patch I submitted last year that I would like to see
>> applied to TFTP. Currently if a transfer fails, TFTP leaves around a bogus
>> file. This patch removes it on failure.
Horst's patch adds imo needless size.
The attached patch takes Jason's and Horst's patches, cleans up the
style (simple ident with the default style) and removes some cruft
(which was optimized away by my compiler anyway, but just did look
wrong) and also removes some stuff which did add to the size without any
apparent benefit (the cached cmd_{get,put} adds just bloat for me).
Not yet finished, missing bits:
- check for argv[optind + 1 == NULL
- see if there is opportunity to make tftp() smaller.
- peruse USE_
- see if perusing ENABLE_ in the code (not the cpp) is a good idea,
size-wise
Could someone elaborate on why that fileno() call was there?
Completely untested so far..
I'll come back and ask folks to test this when i'm done looking over
the todo list above (unless somebody beats me to it, of course :)
No numbers since it's not yet finished..
-------------- next part --------------
Index: networking/tftp.c
===================================================================
--- networking/tftp.c (revision 14938)
+++ networking/tftp.c (working copy)
@@ -34,10 +34,13 @@
#include "busybox.h"
-//#define CONFIG_FEATURE_TFTP_DEBUG
-#define TFTP_BLOCKSIZE_DEFAULT 512 /* according to RFC 1350, don't change */
-#define TFTP_TIMEOUT 5 /* seconds */
+#define TFTP_BLOCKSIZE_DEFAULT 512 /* according to RFC 1350, don't change */
+#define TFTP_TIMEOUT 5 /* seconds */
+
+/* RFC2348 says between 8 and 65464 */
+#define TFTP_OCTECTS_MIN 8
+#define TFTP_OCTECTS_MAX 65464
/* opcodes we support */
@@ -48,7 +51,7 @@
#define TFTP_ERROR 5
#define TFTP_OACK 6
-static const char * const tftp_bb_error_msg[] = {
+static const char *const tftp_bb_error_msg[] = {
"Undefined error",
"File not found",
"Access violation",
@@ -59,13 +62,10 @@ static const char * const tftp_bb_error_
"No such user"
};
-#ifdef CONFIG_FEATURE_TFTP_GET
-# define tftp_cmd_get 1
-#else
-# define tftp_cmd_get 0
-#endif
-#ifdef CONFIG_FEATURE_TFTP_PUT
-# define tftp_cmd_put (tftp_cmd_get+1)
+#define tftp_cmd_get ENABLE_FEATURE_TFTP_GET
+
+#if ENABLE_FEATURE_TFTP_PUT
+# define tftp_cmd_put (tftp_cmd_get+ENABLE_FEATURE_TFTP_PUT)
#else
# define tftp_cmd_put 0
#endif
@@ -82,9 +82,9 @@ static int tftp_blocksize_check(int bloc
*/
if ((bufsize && (blocksize > bufsize)) ||
- (blocksize < 8) || (blocksize > 65464)) {
- bb_error_msg("bad blocksize");
- return 0;
+ (blocksize < TFTP_OCTECTS_MIN) || (blocksize > TFTP_OCTECTS_MAX)) {
+ bb_error_msg("bad blocksize");
+ return 0;
}
return blocksize;
@@ -98,25 +98,24 @@ static char *tftp_option_get(char *buf,
while (len > 0) {
- /* Make sure the options are terminated correctly */
+ /* Make sure the options are terminated correctly */
- for (k = 0; k < len; k++) {
- if (buf[k] == '\0') {
- break;
+ for (k = 0; k < len; k++) {
+ if (buf[k] == '\0') {
+ break;
}
}
if (k >= len) {
- break;
+ break;
}
if (opt_val == 0) {
if (strcasecmp(buf, option) == 0) {
- opt_found = 1;
+ opt_found = 1;
}
- }
- else {
- if (opt_found) {
+ } else {
+ if (opt_found) {
return buf;
}
}
@@ -135,12 +134,10 @@ static char *tftp_option_get(char *buf,
#endif
static inline int tftp(const int cmd, const struct hostent *host,
- const char *remotefile, int localfd, const unsigned short port, int tftp_bufsize)
+ const char *remotefile, int localfd,
+ const unsigned short port, int tftp_bufsize)
{
- const int cmd_get = cmd & tftp_cmd_get;
- const int cmd_put = cmd & tftp_cmd_put;
- const int bb_tftp_num_retries = 5;
-
+# define bb_tftp_num_retries 5
struct sockaddr_in sa;
struct sockaddr_in from;
struct timeval tv;
@@ -155,17 +152,13 @@ static inline int tftp(const int cmd, co
int timeout = bb_tftp_num_retries;
unsigned short block_nr = 1;
-#ifdef CONFIG_FEATURE_TFTP_BLOCKSIZE
- int want_option_ack = 0;
-#endif
+ USE_FEATURE_TFTP_BLOCKSIZE(int want_option_ack = 0;)
/* Can't use RESERVE_CONFIG_BUFFER here since the allocation
* size varies meaning BUFFERS_GO_ON_STACK would fail */
- char *buf=xmalloc(tftp_bufsize + 4);
-
- tftp_bufsize += 4;
+ char *buf = xmalloc(tftp_bufsize += 4);
- if ((socketfd = socket(PF_INET, SOCK_DGRAM, 0)) < 0) { /* bb_xsocket? */
+ if ((socketfd = socket(PF_INET, SOCK_DGRAM, 0)) < 0) { /* bb_xsocket? */
bb_perror_msg("socket");
return EXIT_FAILURE;
}
@@ -173,7 +166,7 @@ static inline int tftp(const int cmd, co
len = sizeof(sa);
memset(&sa, 0, len);
- bind(socketfd, (struct sockaddr *)&sa, len);
+ bb_xbind(socketfd, (struct sockaddr *) &sa, len);
sa.sin_family = host->h_addrtype;
sa.sin_port = port;
@@ -182,11 +175,11 @@ static inline int tftp(const int cmd, co
/* build opcode */
- if (cmd_get) {
+ if (cmd & tftp_cmd_get) {
opcode = TFTP_RRQ;
}
- if (cmd_put) {
+ if (cmd & tftp_cmd_put) {
opcode = TFTP_WRQ;
}
@@ -202,8 +195,8 @@ static inline int tftp(const int cmd, co
/* add filename and mode */
- if ((cmd_get && (opcode == TFTP_RRQ)) ||
- (cmd_put && (opcode == TFTP_WRQ))) {
+ if (((cmd & tftp_cmd_get) && (opcode == TFTP_RRQ)) ||
+ ((cmd & tftp_cmd_put) && (opcode == TFTP_WRQ))) {
int too_long = 0;
/* see if the filename fits into buf */
@@ -212,10 +205,9 @@ static inline int tftp(const int cmd, co
len = strlen(remotefile) + 1;
if ((cp + len) >= &buf[tftp_bufsize - 1]) {
- too_long = 1;
- }
- else {
- safe_strncpy(cp, remotefile, len);
+ too_long = 1;
+ } else {
+ safe_strncpy(cp, remotefile, len);
cp += len;
}
@@ -231,12 +223,12 @@ static inline int tftp(const int cmd, co
#ifdef CONFIG_FEATURE_TFTP_BLOCKSIZE
- len = tftp_bufsize - 4; /* data block size */
+ len = tftp_bufsize - 4; /* data block size */
if (len != TFTP_BLOCKSIZE_DEFAULT) {
- if ((&buf[tftp_bufsize - 1] - cp) < 15) {
- bb_error_msg("too long remote-filename");
+ if ((&buf[tftp_bufsize - 1] - cp) < 15) {
+ bb_error_msg("too long remote-filename");
break;
}
@@ -254,8 +246,8 @@ static inline int tftp(const int cmd, co
/* add ack and data */
- if ((cmd_get && (opcode == TFTP_ACK)) ||
- (cmd_put && (opcode == TFTP_DATA))) {
+ if (((cmd & tftp_cmd_get) && (opcode == TFTP_ACK)) ||
+ ((cmd & tftp_cmd_put) && (opcode == TFTP_DATA))) {
*((unsigned short *) cp) = htons(block_nr);
@@ -263,7 +255,7 @@ static inline int tftp(const int cmd, co
block_nr++;
- if (cmd_put && (opcode == TFTP_DATA)) {
+ if ((cmd & tftp_cmd_put) && (opcode == TFTP_DATA)) {
len = bb_full_read(localfd, cp, tftp_bufsize - 4);
if (len < 0) {
@@ -283,7 +275,7 @@ static inline int tftp(const int cmd, co
/* send packet */
- timeout = bb_tftp_num_retries; /* re-initialize */
+ timeout = bb_tftp_num_retries; /* re-initialize */
do {
len = cp - buf;
@@ -291,11 +283,11 @@ static inline int tftp(const int cmd, co
#ifdef CONFIG_FEATURE_TFTP_DEBUG
fprintf(stderr, "sending %u bytes\n", len);
for (cp = buf; cp < &buf[len]; cp++)
- fprintf(stderr, "%02x ", (unsigned char)*cp);
+ fprintf(stderr, "%02x ", (unsigned char) *cp);
fprintf(stderr, "\n");
#endif
if (sendto(socketfd, buf, len, 0,
- (struct sockaddr *) &sa, sizeof(sa)) < 0) {
+ (struct sockaddr *) &sa, sizeof(sa)) < 0) {
bb_perror_msg("send");
len = -1;
break;
@@ -320,7 +312,7 @@ static inline int tftp(const int cmd, co
switch (select(socketfd + 1, &rfds, NULL, NULL, &tv)) {
case 1:
len = recvfrom(socketfd, buf, tftp_bufsize, 0,
- (struct sockaddr *) &from, &fromlen);
+ (struct sockaddr *) &from, &fromlen);
if (len < 0) {
bb_perror_msg("recvfrom");
@@ -378,7 +370,7 @@ static inline int tftp(const int cmd, co
msg = &buf[4];
buf[tftp_bufsize - 1] = '\0';
} else if (tmp < (sizeof(tftp_bb_error_msg)
- / sizeof(char *))) {
+ / sizeof(char *))) {
msg = tftp_bb_error_msg[tmp];
}
@@ -389,55 +381,51 @@ static inline int tftp(const int cmd, co
break;
}
-
#ifdef CONFIG_FEATURE_TFTP_BLOCKSIZE
if (want_option_ack) {
- want_option_ack = 0;
+ want_option_ack = 0;
- if (opcode == TFTP_OACK) {
+ if (opcode == TFTP_OACK) {
- /* server seems to support options */
+ /* server seems to support options */
- char *res;
+ char *res;
- res = tftp_option_get(&buf[2], len-2,
- "blksize");
+ res = tftp_option_get(&buf[2], len - 2, "blksize");
- if (res) {
- int blksize = atoi(res);
+ if (res) {
+ int blksize = atoi(res);
- if (tftp_blocksize_check(blksize,
- tftp_bufsize - 4)) {
+ if (tftp_blocksize_check(blksize, tftp_bufsize - 4)) {
- if (cmd_put) {
- opcode = TFTP_DATA;
- }
- else {
- opcode = TFTP_ACK;
- }
+ if (cmd & tftp_cmd_put) {
+ opcode = TFTP_DATA;
+ } else {
+ opcode = TFTP_ACK;
+ }
#ifdef CONFIG_FEATURE_TFTP_DEBUG
- fprintf(stderr, "using blksize %u\n", blksize);
+ fprintf(stderr, "using blksize %u\n", blksize);
#endif
- tftp_bufsize = blksize + 4;
- block_nr = 0;
- continue;
- }
- }
- /* FIXME:
- * we should send ERROR 8 */
- bb_error_msg("bad server option");
- break;
- }
+ tftp_bufsize = blksize + 4;
+ block_nr = 0;
+ continue;
+ }
+ }
+ /* FIXME:
+ * we should send ERROR 8 */
+ bb_error_msg("bad server option");
+ break;
+ }
- bb_error_msg("warning: blksize not supported by server"
- " - reverting to 512");
+ bb_error_msg("warning: blksize not supported by server"
+ " - reverting to 512");
- tftp_bufsize = TFTP_BLOCKSIZE_DEFAULT + 4;
+ tftp_bufsize = TFTP_BLOCKSIZE_DEFAULT + 4;
}
#endif
- if (cmd_get && (opcode == TFTP_DATA)) {
+ if ((cmd & tftp_cmd_get) && (opcode == TFTP_DATA)) {
if (tmp == block_nr) {
@@ -456,7 +444,7 @@ static inline int tftp(const int cmd, co
continue;
}
/* in case the last ack disappeared into the ether */
- if ( tmp == (block_nr - 1) ) {
+ if (tmp == (block_nr - 1)) {
--block_nr;
opcode = TFTP_ACK;
continue;
@@ -468,9 +456,9 @@ static inline int tftp(const int cmd, co
}
}
- if (cmd_put && (opcode == TFTP_ACK)) {
+ if ((cmd & tftp_cmd_put) && (opcode == TFTP_ACK)) {
- if (tmp == (unsigned short)(block_nr - 1)) {
+ if (tmp == (unsigned short) (block_nr - 1)) {
if (finished) {
break;
}
@@ -483,7 +471,6 @@ static inline int tftp(const int cmd, co
#ifdef CONFIG_FEATURE_CLEAN_UP
close(socketfd);
-
free(buf);
#endif
@@ -506,6 +493,7 @@ int tftp_main(int argc, char **argv)
#ifdef CONFIG_FEATURE_TFTP_BLOCKSIZE
char *sblocksize = NULL;
+
#define BS "b:"
#define BS_ARG , &sblocksize
#else
@@ -539,35 +527,39 @@ int tftp_main(int argc, char **argv)
cmd = bb_getopt_ulflags(argc, argv, GET PUT "l:r:" BS,
- &localfile, &remotefile BS_ARG);
-#ifdef CONFIG_FEATURE_TFTP_BLOCKSIZE
- if(sblocksize) {
- blocksize = atoi(sblocksize);
- if (!tftp_blocksize_check(blocksize, 0)) {
- return EXIT_FAILURE;
- }
- }
-#endif
+ &localfile, &remotefile BS_ARG);
cmd &= (tftp_cmd_get | tftp_cmd_put);
#ifdef CONFIG_FEATURE_TFTP_GET
- if(cmd == tftp_cmd_get)
+ if (cmd == tftp_cmd_get)
flags = O_WRONLY | O_CREAT | O_TRUNC;
#endif
#ifdef CONFIG_FEATURE_TFTP_PUT
- if(cmd == tftp_cmd_put)
+ if (cmd == tftp_cmd_put)
flags = O_RDONLY;
#endif
- if(localfile == NULL)
- localfile = remotefile;
- if(remotefile == NULL)
- remotefile = localfile;
+#ifdef CONFIG_FEATURE_TFTP_BLOCKSIZE
+ if (sblocksize) {
+ blocksize = atoi(sblocksize);
+ if (!tftp_blocksize_check(blocksize, 0)) {
+ return EXIT_FAILURE;
+ }
+ }
+#endif
+
+ if (localfile == NULL)
+ localfile = remotefile;
+ if (remotefile == NULL)
+ remotefile = localfile;
+ if ((localfile == NULL && remotefile == NULL) || (argv[optind] == NULL))
+ bb_show_usage();
/* XXX: I corrected this, but may be wrong too. vodz */
- if(localfile==NULL || strcmp(localfile, "-") == 0) {
- fd = fileno((cmd==tftp_cmd_get)? stdout : stdin);
- } else if (fd==-1) {
- fd = open(localfile, flags, 0644);
+ if (localfile == NULL || strcmp(localfile, "-") == 0) {
+/*huh? fd = fileno((cmd == tftp_cmd_get) ? stdout : stdin); */
+ fd = (cmd == tftp_cmd_get) ? STDOUT_FILENO : STDIN_FILENO;
+ } else {
+ fd = open(localfile, flags, 0644); /* fail below */
}
if (fd < 0) {
bb_perror_msg_and_die("local file");
@@ -579,17 +571,19 @@ int tftp_main(int argc, char **argv)
#ifdef CONFIG_FEATURE_TFTP_DEBUG
fprintf(stderr, "using server \"%s\", remotefile \"%s\", "
- "localfile \"%s\".\n",
- inet_ntoa(*((struct in_addr *) host->h_addr)),
- remotefile, localfile);
+ "localfile \"%s\".\n",
+ inet_ntoa(*((struct in_addr *) host->h_addr)),
+ remotefile, localfile);
#endif
result = tftp(cmd, host, remotefile, fd, port, blocksize);
#ifdef CONFIG_FEATURE_CLEAN_UP
if (!(fd == STDOUT_FILENO || fd == STDIN_FILENO)) {
- close(fd);
+ close(fd);
+ if (cmd == tftp_cmd_get && result != EXIT_SUCCESS)
+ unlink(localfile);
}
#endif
- return(result);
+ return (result);
}
More information about the busybox
mailing list