Discussion:
[PATCH] Fix 2 possible SEGVs in tftp client
KRONSTORFER Horst
2006-04-21 12:04:26 UTC
Permalink
Skipped content of type multipart/alternative-------------- next part --------------
A non-text attachment was scrubbed...
Name: tftp.patch
Type: application/octet-stream
Size: 793 bytes
Desc: tftp.patch
Url : http://busybox.net/lists/busybox/attachments/20060421/8a027a4c/tftp.obj
Jason Schoon
2006-04-21 12:46:17 UTC
Permalink
Skipped content of type multipart/alternative-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox_tftp_remove_file.patch
Type: text/x-patch
Size: 513 bytes
Desc: not available
Url : http://busybox.net/lists/busybox/attachments/20060421/a5c89983/busybox_tftp_remove_file.bin
KRONSTORFER Horst
2006-04-21 13:06:13 UTC
Permalink
-----Original Message-----
From: Jason Schoon [mailto:***@gmail.com]
Sent: Fri 4/21/2006 4:46 PM
To: KRONSTORFER Horst
Cc: ***@busybox.net
Subject: Re: [PATCH] Fix 2 possible SEGVs in tftp client
hi!
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
/* 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://busybox.net/lists/busybox/attachments/20060421/57d59dad/attachment.htm
Bernhard Fischer
2006-04-22 09:14:21 UTC
Permalink
hi!
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
/* 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);
}
Jason Schoon
2006-04-24 18:26:56 UTC
Permalink
Skipped content of type multipart/alternative-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox.tftp_touchup.04.patch
Type: text/x-patch
Size: 13266 bytes
Desc: not available
Url : http://busybox.net/lists/busybox/attachments/20060424/fe28a2d8/busybox.tftp_touchup.04.bin
Bernhard Fischer
2006-06-10 11:24:16 UTC
Permalink
Post by Jason Schoon
Post by Bernhard Fischer
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).
- 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..
Here is an updated patch. First, I realized I had my file cleanup code in
the wrong place. It should be called unconditionally, not in
FEATURE_CLEANUP. I then went through and cleaned up some more of the code a
bit, removing useless comments and magic numbers as best I could. I did not
yet try to shrink tftp() or switch to the ENABLE_ macros.
I'm currently looking at opportunities to shrink it a bit (on top of our
cummulative 04 patch).
Post by Jason Schoon
--- busybox/networking/tftp.c 2006-04-24 15:22:35.000000000 -0500
+++ busybox/networking/tftp.c.mine 2006-04-24 15:22:05.000000000 -0500
@@ -34,13 +34,22 @@
+static const char OPTION_BLOCKSIZE[] = "blocksize";
+static unsigned char OPTION_BLOCKSIZE_LEN = sizeof(OPTION_BLOCKSIZE);
- memcpy(cp, "blksize", 8);
- cp += 8;
-
+ memcpy(cp, OPTION_BLOCKSIZE, OPTION_BLOCKSIZE_LEN);
+ cp += OPTION_BLOCKSIZE_LEN;
cp += snprintf(cp, 6, "%d", len) + 1;
That typo i managed to introduce in the 03 patch will of course not be
in the final patch ("blocksize" != "blksize").

Did anyone look at that IPv6-support patch for tftp in the bugtracker?



Also there is a patch which fixes a bug on bad packets
(http://bugs.busybox.net/view.php?id=634). The proposed patch looks like
it overdoes it a bit. Perhaps it would be better to just reset the bad
packet completely before continuing, have to look if that is feasable.
Thoughts?
Bernhard Fischer
2006-06-10 12:21:23 UTC
Permalink
Post by Bernhard Fischer
Post by Jason Schoon
Post by Bernhard Fischer
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).
- 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..
Here is an updated patch. First, I realized I had my file cleanup code in
the wrong place. It should be called unconditionally, not in
FEATURE_CLEANUP. I then went through and cleaned up some more of the code a
bit, removing useless comments and magic numbers as best I could. I did not
yet try to shrink tftp() or switch to the ENABLE_ macros.
I'm currently looking at opportunities to shrink it a bit (on top of our
cummulative 04 patch).
Applied as http://busybox.net/cgi-bin/viewcvs.cgi?rev=15355&view=rev I
think I didn't damaged it too much..
Thanks to anyone who was involved.
cheers,
Bernhard
Jason Schoon
2006-06-10 15:00:05 UTC
Permalink
Post by Bernhard Fischer
Applied as http://busybox.net/cgi-bin/viewcvs.cgi?rev=15355&view=rev I
think I didn't damaged it too much..
Thanks to anyone who was involved.
cheers,
Bernhard
Looks good, I'll have to give a try when I am back in front of my system. I
plan to take a look at the IPv6 patch and massage it a bit. Telnet client,
remote syslog, ftpputget are also on my list of planned conversions to IPv6.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://busybox.net/lists/busybox/attachments/20060610/c917621d/attachment.html
Jason Schoon
2006-06-13 05:21:00 UTC
Permalink
Post by Bernhard Fischer
Post by Bernhard Fischer
Post by Jason Schoon
Post by Bernhard Fischer
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).
- 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..
Here is an updated patch. First, I realized I had my file cleanup code in
the wrong place. It should be called unconditionally, not in
FEATURE_CLEANUP. I then went through and cleaned up some more of the code a
bit, removing useless comments and magic numbers as best I could. I did not
yet try to shrink tftp() or switch to the ENABLE_ macros.
I'm currently looking at opportunities to shrink it a bit (on top of our
cummulative 04 patch).
Applied as http://busybox.net/cgi-bin/viewcvs.cgi?rev=15355&view=rev I
think I didn't damaged it too much..
Thanks to anyone who was involved.
cheers,
Bernhard
Attached is one more patch for this to fix up two minor issues. A
duplicate const declaration, and my error in not checking if we were
using stdin or stdout before freeing the file handle.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox_tftp.patch
Type: text/x-patch
Size: 777 bytes
Desc: not available
Url : http://busybox.net/lists/busybox/attachments/20060613/0421c7d1/busybox_tftp.bin
Bernhard Fischer
2006-06-13 10:26:29 UTC
Permalink
Post by Jason Schoon
Attached is one more patch for this to fix up two minor issues. A
duplicate const declaration, and my error in not checking if we were
using stdin or stdout before freeing the file handle.
--- busybox/networking/tftp.c.orig 2006-06-13 02:12:44.000000000 -0500
+++ busybox/networking/tftp.c 2006-06-13 02:13:55.000000000 -0500
@@ -95,7 +95,7 @@
return blocksize;
}
-static char *tftp_option_get(char *buf, int len, const char const *option)
->+static char *tftp_option_get(char *buf, int len, char const *option)
+static char *tftp_option_get(char *buf, int len, const char * const option)
Post by Jason Schoon
{
int opt_val = 0;
int opt_found = 0;
@@ -573,12 +573,12 @@
result = tftp(cmd, host, remotefile, fd, port, blocksize);
-#ifdef CONFIG_FEATURE_CLEAN_UP
if (!(fd == STDOUT_FILENO || fd == STDIN_FILENO)) {
+#ifdef CONFIG_FEATURE_CLEAN_UP
Do i understand you right?
if (ENABLE_FEATURE_CLEAN_UP) {
if (!(fd == STDOUT_FILENO || fd == STDIN_FILENO))
close(fd);
if (cmd == tftp_cmd_get && result != EXIT_SUCCESS)
unlink(localfile);
}
Why do you want to bypass feature_cleanup for rm'ing the failed file?
Just curious..
Jason Schoon
2006-06-13 12:28:50 UTC
Permalink
Post by Bernhard Fischer
Post by Jason Schoon
Attached is one more patch for this to fix up two minor issues. A
duplicate const declaration, and my error in not checking if we were
using stdin or stdout before freeing the file handle.
--- busybox/networking/tftp.c.orig 2006-06-13 02:12:44.000000000 -0500
+++ busybox/networking/tftp.c 2006-06-13 02:13:55.000000000 -0500
@@ -95,7 +95,7 @@
return blocksize;
}
-static char *tftp_option_get(char *buf, int len, const char const *option)
->+static char *tftp_option_get(char *buf, int len, char const *option)
+static char *tftp_option_get(char *buf, int len, const char * const option)
Post by Jason Schoon
{
int opt_val = 0;
int opt_found = 0;
@@ -573,12 +573,12 @@
result = tftp(cmd, host, remotefile, fd, port, blocksize);
-#ifdef CONFIG_FEATURE_CLEAN_UP
if (!(fd == STDOUT_FILENO || fd == STDIN_FILENO)) {
+#ifdef CONFIG_FEATURE_CLEAN_UP
Do i understand you right?
if (ENABLE_FEATURE_CLEAN_UP) {
if (!(fd == STDOUT_FILENO || fd == STDIN_FILENO))
close(fd);
if (cmd == tftp_cmd_get && result != EXIT_SUCCESS)
unlink(localfile);
}
Why do you want to bypass feature_cleanup for rm'ing the failed file?
Just curious..
I'll admit it was late when I made those changes, but I'm confused
now. The feature_cleanup stuff shouldn't have changed, I just moved
the check for stdin/stdout outside of that ifdef because we want to
unlink the file in any error case, regardless of feature_cleanup being
enabled. Moving the check outside of feature_cleanup saves checking
the file descriptors twice.

Checking....Yeah, it appears to do what I want. Is there something
else I am missing?

When I grabbed the patch back off the mailing list (no local copy
here), it didn't seem to apply completely cleanly. It possible either
my browser or something else horked it, but attached is one that I
just verified patches cleanly, just in case.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox_tftp.patch
Type: text/x-patch
Size: 777 bytes
Desc: not available
Url : http://busybox.net/lists/busybox/attachments/20060613/f569e4cc/busybox_tftp.bin
Erik Hovland
2006-06-13 13:31:05 UTC
Permalink
--- busybox/networking/tftp.c 2006-06-13 09:26:45.000000000 -0500
+++ busybox/networking/tftp.c.mine 2006-06-13 09:26:11.000000000 -0500
@@ -95,7 +95,7 @@
return blocksize;
}
-static char *tftp_option_get(char *buf, int len, const char const *option)
+static char *tftp_option_get(char *buf, int len, char const *option)
You could write this as:
static char *tftp_option_get(char *buf, int len, const char *const option)

The const in the right place would allow the compiler to further
optimize the case where both the dereferenced string and the pointer to
the string are not changed in the function that they are being passed
to. Not sure if that adds to a real space savings.

See my post and patch to utils.c and utils.h where I did this for
another function:
http://busybox.net/lists/busybox/2006-June/022287.html

E
--
Erik Hovland
mail: ***@hovland.org
web: http://hovland.org/
PGP/GPG public key available on request
Jason Schoon
2006-06-13 14:55:30 UTC
Permalink
Post by Jason Schoon
--- busybox/networking/tftp.c 2006-06-13 09:26:45.000000000 -0500
+++ busybox/networking/tftp.c.mine 2006-06-13 09:26:11.000000000-0500
@@ -95,7 +95,7 @@
return blocksize;
}
-static char *tftp_option_get(char *buf, int len, const char const
*option)
+static char *tftp_option_get(char *buf, int len, char const *option)
static char *tftp_option_get(char *buf, int len, const char *const option)
The const in the right place would allow the compiler to further
optimize the case where both the dereferenced string and the pointer to
the string are not changed in the function that they are being passed
to. Not sure if that adds to a real space savings.
See my post and patch to utils.c and utils.h where I did this for
http://busybox.net/lists/busybox/2006-June/022287.html
I see that now, I thought it was a typo since my compiler threw a warning
about it. If there are valid space savings for it, we need to find a way to
suppress the warning.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://busybox.net/lists/busybox/attachments/20060613/742ff7d5/attachment.html
Erik Hovland
2006-06-14 01:19:56 UTC
Permalink
Post by Jason Schoon
Post by Jason Schoon
--- busybox/networking/tftp.c 2006-06-13 09:26:45.000000000 -0500
+++ busybox/networking/tftp.c.mine 2006-06-13 09:26:11.000000000-0500
@@ -95,7 +95,7 @@
return blocksize;
}
-static char *tftp_option_get(char *buf, int len, const char const
*option)
+static char *tftp_option_get(char *buf, int len, char const *option)
static char *tftp_option_get(char *buf, int len, const char *const option)
The const in the right place would allow the compiler to further
optimize the case where both the dereferenced string and the pointer to
the string are not changed in the function that they are being passed
to. Not sure if that adds to a real space savings.
See my post and patch to utils.c and utils.h where I did this for
http://busybox.net/lists/busybox/2006-June/022287.html
I see that now, I thought it was a typo since my compiler threw a warning
about it. If there are valid space savings for it, we need to find a way to
suppress the warning.
If you want to make the string const, put the const before the *.

If you want to make the pointer that points to the string const, put the
const after the *.

const char* foo = bb_xstrdup(bar); // string (data) is const
char* const foo = bb_xstrdup(bar); // pointer to string is const

const char *const foo = bb_xstrdup(bar); // both string and pointer are
// const.

const char const* foo = bb_xstrdup(bar); // Illegal in C99
// warning with older gcc
// error with newer gcc

E
--
Erik Hovland
mail: ***@hovland.org
web: http://hovland.org/
PGP/GPG public key available on request
Rob Landley
2006-06-17 21:55:11 UTC
Permalink
Post by Erik Hovland
Post by Jason Schoon
Post by Jason Schoon
--- busybox/networking/tftp.c 2006-06-13 09:26:45.000000000 -0500
+++ busybox/networking/tftp.c.mine 2006-06-13
return blocksize;
}
-static char *tftp_option_get(char *buf, int len, const char const
*option)
+static char *tftp_option_get(char *buf, int len, char const *option)
static char *tftp_option_get(char *buf, int len, const char *const option)
The const in the right place would allow the compiler to further
optimize the case where both the dereferenced string and the pointer to
the string are not changed in the function that they are being passed
to. Not sure if that adds to a real space savings.
See my post and patch to utils.c and utils.h where I did this for
http://busybox.net/lists/busybox/2006-June/022287.html
I see that now, I thought it was a typo since my compiler threw a warning
about it. If there are valid space savings for it, we need to find a way
to suppress the warning.
If you want to make the string const, put the const before the *.
In build-at-once mode the compiler _really_ should be able to figure this
out...

Rob
--
Never bet against the cheap plastic solution.
Rob Landley
2006-06-17 21:57:54 UTC
Permalink
Post by Erik Hovland
Post by Jason Schoon
Post by Erik Hovland
See my post and patch to utils.c and utils.h where I did this for
http://busybox.net/lists/busybox/2006-June/022287.html
I see that now, I thought it was a typo since my compiler threw a warning
about it. If there are valid space savings for it, we need to find a way
to suppress the warning.
If you want to make the string const, put the const before the *.
Also, when we have to typecast our way around this crap (as in
skip_whitespace() in libbb where half the users typecast the return value
back to a normal char * when they use it, but it _is_ a normal char * to
begin with...)

I'm not a fan of const. Really.

Rob
--
Never bet against the cheap plastic solution.
Erik Hovland
2006-06-17 23:07:54 UTC
Permalink
Post by Rob Landley
Post by Erik Hovland
Post by Jason Schoon
Post by Erik Hovland
See my post and patch to utils.c and utils.h where I did this for
http://busybox.net/lists/busybox/2006-June/022287.html
I see that now, I thought it was a typo since my compiler threw a warning
about it. If there are valid space savings for it, we need to find a way
to suppress the warning.
If you want to make the string const, put the const before the *.
Also, when we have to typecast our way around this crap (as in
skip_whitespace() in libbb where half the users typecast the return value
back to a normal char * when they use it, but it _is_ a normal char * to
begin with...)
I'm not a fan of const. Really.
I think that the compiler could care a flip about whether you are a
fanatic of const. I could really care less about const either. I think
you probably agree that there is only one thing that will decide whether
const should be used or not, size of binary.

But if one is going to use const, they should do it right and at least
understand what the placement of const regarding the * operator does to
the string. I have submitted two patches in so many weeks that
corrected const. Both came after commits that put const in the wrong
spot and generated warnings. I think we all could do without those sorts
of mistakes so we can get to doing the work we want to do.

E
--
Erik Hovland
mail: ***@hovland.org
web: http://hovland.org/
PGP/GPG public key available on request
Rob Landley
2006-06-18 14:24:25 UTC
Permalink
Post by Erik Hovland
Post by Rob Landley
Post by Erik Hovland
Post by Jason Schoon
Post by Erik Hovland
See my post and patch to utils.c and utils.h where I did this for
http://busybox.net/lists/busybox/2006-June/022287.html
I see that now, I thought it was a typo since my compiler threw a
warning about it. If there are valid space savings for it, we need
to find a way to suppress the warning.
If you want to make the string const, put the const before the *.
Also, when we have to typecast our way around this crap (as in
skip_whitespace() in libbb where half the users typecast the return value
back to a normal char * when they use it, but it _is_ a normal char * to
begin with...)
I'm not a fan of const. Really.
I think that the compiler could care a flip about whether you are a
fanatic of const.
The words "fan" and "fanatic" have diverged rather a lot in the last century.
And "terrific" no longer means "inspiring terror". "Spectacular" is still
related to drawing spectators, but has other connotations as well.

Just FYI.
Post by Erik Hovland
I could really care less about const either. I think
you probably agree that there is only one thing that will decide whether
const should be used or not, size of binary.
On which compiler version? If it's an optimization for gcc 2.95 but has no
effect on gcc 4.1, which is likely to be still relevant in 3 years?
Post by Erik Hovland
But if one is going to use const, they should do it right and at least
understand what the placement of const regarding the * operator does to
the string.
One reason I dislike is it having to understand (and maintain) non-obvious
rules like that.
Post by Erik Hovland
I have submitted two patches in so many weeks that
corrected const.
Needing to do this strikes me as a bad thing. Do current compilers benefit
from this, or just old ones? And by how much? This never came up in the
discussion, that I'm aware of...
Post by Erik Hovland
Both came after commits that put const in the wrong
spot and generated warnings. I think we all could do without those sorts
of mistakes so we can get to doing the work we want to do.
Yeah. Avoid using const.

I just threw "#define const" at the top of libbb.h, and fixed up half a dozen
files that broke when I did that (because they didn't #include libbb.h at the
top), and the result was actually 1500 bytes _smaller_. By not having
anything say const anywhere. (The resulting binary was about 2k smaller as
well.)

Just FYI. With gcc 4.0.3.
Post by Erik Hovland
E
Rob
--
Never bet against the cheap plastic solution.
Erik Hovland
2006-06-18 15:24:58 UTC
Permalink
Post by Rob Landley
Post by Erik Hovland
Post by Rob Landley
Post by Erik Hovland
Post by Jason Schoon
Post by Erik Hovland
See my post and patch to utils.c and utils.h where I did this for
http://busybox.net/lists/busybox/2006-June/022287.html
I see that now, I thought it was a typo since my compiler threw a
warning about it. If there are valid space savings for it, we need
to find a way to suppress the warning.
If you want to make the string const, put the const before the *.
Also, when we have to typecast our way around this crap (as in
skip_whitespace() in libbb where half the users typecast the return value
back to a normal char * when they use it, but it _is_ a normal char * to
begin with...)
I'm not a fan of const. Really.
I think that the compiler could care a flip about whether you are a
fanatic of const.
The words "fan" and "fanatic" have diverged rather a lot in the last century.
And "terrific" no longer means "inspiring terror". "Spectacular" is still
related to drawing spectators, but has other connotations as well.
Just FYI.
Mr. Landley, you are incredibly pugnacious. It makes for humorous
reading.

E
--
Erik Hovland
mail: ***@hovland.org
web: http://hovland.org/
PGP/GPG public key available on request
Rob Landley
2006-06-18 22:00:40 UTC
Permalink
Post by Erik Hovland
Post by Rob Landley
Post by Erik Hovland
I think that the compiler could care a flip about whether you are a
fanatic of const.
The words "fan" and "fanatic" have diverged rather a lot in the last
century. And "terrific" no longer means "inspiring terror".
"Spectacular" is still related to drawing spectators, but has other
connotations as well.
Just FYI.
Mr. Landley, you are incredibly pugnacious. It makes for humorous
reading.
I've been getting irritable way too easily lately. Sorry 'bout that.

Part of it is that I'm used to summer in Austin (where air conditioning is
considered important) as opposed to Pittsburgh (where it's sort of an
optional extra, with a single wall unit for an entire apartment. Also the
humidity here gets high enough I wind up taking showers to dry off.)

I'm also noticing that my current to-do list involves "oh, I was in the middle
of reviewing the mdev shellout patch when Bernhard submitted his lash cleanup
thing (_after_ he feature freeze, and it's not a bug fix) and pestered me
until I put the mdev thing on hold and started messing with lash, and now
here's another tangent with "const" when I'm about halfway through the lash
patch. I suppose I'd be done with the lash patch already if I hadn't visited
ikea, put together a desk, caught up on my sleep since it's the weekend...
That was my bad...)

Apparently, this is what project maintenance is like. (I keep wanting to snap
at people "stop submitting patches and bringing up new topics to the list
until I've dealt with the backlog!" but it just doesn't work that way...)

Right _now_, the tangent is somebody just dumped a "make stty bigger" patch
(svn 15412) unannounced and unconfigurable (although it's a bit small for a
config option) which that adds tons of #ifdefs to already dirty code. I feel
compelled to clean up after this, since the last really major set of patches
from this committer was the e2fsprogs directory.

I'd bounce the cleanup off of danf to make sure it still works for him, but
the checkin never said who that was. I personally consider it bad form (from
a copyright perspective, among other things) to commit patches that are
copyright "who knows", but I'm funny that way...

Anyway, that's why I just checked in svn 15421, which I don't currently have a
test case for and is in an area of code I don't want go spend more time on
right now. (I'd be much happier if I didn't have to care about this sort of
thing can could focus on making one area at a time better...)
Post by Erik Hovland
E
Rob
--
Never bet against the cheap plastic solution.
Rob Landley
2006-06-19 00:00:44 UTC
Permalink
Post by Erik Hovland
Post by Rob Landley
Post by Erik Hovland
I think that the compiler could care a flip about whether you are a
fanatic of const.
The words "fan" and "fanatic" have diverged rather a lot in the last
century. And "terrific" no longer means "inspiring terror".
"Spectacular" is still related to drawing spectators, but has other
connotations as well.
Just FYI.
Mr. Landley, you are incredibly pugnacious. It makes for humorous
reading.
I've been getting irritable way too easily lately. Sorry 'bout that.

Part of it is that I'm used to summer in Austin (where air conditioning is
considered important) as opposed to Pittsburgh (where it's sort of an
optional extra, with a single wall unit for an entire apartment. Also the
humidity here gets high enough I wind up taking showers to dry off.)

I'm also noticing that my current to-do list involves "oh, I was in the middle
of reviewing the mdev shellout patch when Bernhard submitted his lash cleanup
thing (_after_ he feature freeze, and it's not a bug fix) and pestered me
until I put the mdev thing on hold and started messing with lash, and now
here's another tangent with "const" when I'm about halfway through the lash
patch. I suppose I'd be done with the lash patch already if I hadn't visited
ikea, put together a desk, caught up on my sleep since it's the weekend...
That was my bad...)

Apparently, this is what project maintenance is like. (I keep wanting to snap
at people "stop submitting patches and bringing up new topics to the list
until I've dealt with the backlog!" but it just doesn't work that way...)

Right _now_, the tangent is somebody just dumped a "make stty bigger" patch
(svn 15412) unannounced and unconfigurable (although it's a bit small for a
config option) which that adds tons of #ifdefs to already dirty code. I feel
compelled to clean up after this, since the last really major set of patches
from this committer was the e2fsprogs directory.

I'd bounce the cleanup off of danf to make sure it still works for him, but
the checkin never said who that was. I personally consider it bad form (from
a copyright perspective, among other things) to commit patches that are
copyright "who knows", but I'm funny that way...

Anyway, that's why I just checked in svn 15421, which I don't currently have a
test case for and is in an area of code I don't want go spend more time on
right now. (I'd be much happier if I didn't have to care about this sort of
thing can could focus on making one area at a time better...)
Post by Erik Hovland
E
Rob
--
Never bet against the cheap plastic solution.
Jason Schoon
2006-06-18 15:45:19 UTC
Permalink
Post by Rob Landley
Yeah. Avoid using const.
I just threw "#define const" at the top of libbb.h, and fixed up half a dozen
files that broke when I did that (because they didn't #include libbb.h at the
top), and the result was actually 1500 bytes _smaller_. By not having
anything say const anywhere. (The resulting binary was about 2k smaller as
well.)
Just FYI. With gcc 4.0.3.
E
Rob
--
Never bet against the cheap plastic solution.
_______________________________________________
busybox mailing list
http://busybox.net/cgi-bin/mailman/listinfo/busybox
Running a very similar test with gcc 3.3.4 on x86 (Built clean using
defconfig, then added #define const to the top of libbb.h, then rebuilt
defconfig clean once more, fixing up the few places where header ordering
had caused an error), I actually ended up with a binary that was 14k
smaller.

If const were actually an enforced type in a strongly-typed language, I
might have concern about removing it. However, given that anybody can
simply ignore the const by casting, and that it appears to increase binary
size, not to mention causing unnecessary code fixups, I say whack it.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://busybox.net/lists/busybox/attachments/20060618/4e945334/attachment.html
Rob Landley
2006-06-18 19:19:33 UTC
Permalink
Post by Jason Schoon
Running a very similar test with gcc 3.3.4 on x86 (Built clean using
defconfig, then added #define const to the top of libbb.h, then rebuilt
defconfig clean once more, fixing up the few places where header ordering
had caused an error), I actually ended up with a binary that was 14k
smaller.
So it's not just me then. Good to know. :)
Post by Jason Schoon
If const were actually an enforced type in a strongly-typed language, I
might have concern about removing it. However, given that anybody can
simply ignore the const by casting, and that it appears to increase binary
size, not to mention causing unnecessary code fixups, I say whack it.
I'm amused that the rationale given all this time was that the resulting
binary was smaller if you used const.

Now admittedly, putting some data in the read only section is useful,
especially on nommu machines. But function arguments accepting data off the
stack?

Rob
--
Never bet against the cheap plastic solution.
Rob Landley
2006-06-18 21:19:37 UTC
Permalink
Post by Jason Schoon
Running a very similar test with gcc 3.3.4 on x86 (Built clean using
defconfig, then added #define const to the top of libbb.h, then rebuilt
defconfig clean once more, fixing up the few places where header ordering
had caused an error), I actually ended up with a binary that was 14k
smaller.
So it's not just me then. Good to know. :)
Post by Jason Schoon
If const were actually an enforced type in a strongly-typed language, I
might have concern about removing it. However, given that anybody can
simply ignore the const by casting, and that it appears to increase binary
size, not to mention causing unnecessary code fixups, I say whack it.
I'm amused that the rationale given all this time was that the resulting
binary was smaller if you used const.

Now admittedly, putting some data in the read only section is useful,
especially on nommu machines. But function arguments accepting data off the
stack?

Rob
--
Never bet against the cheap plastic solution.
Erik Hovland
2006-06-18 17:24:35 UTC
Permalink
Post by Rob Landley
Post by Erik Hovland
Post by Rob Landley
Post by Erik Hovland
Post by Jason Schoon
Post by Erik Hovland
See my post and patch to utils.c and utils.h where I did this for
http://busybox.net/lists/busybox/2006-June/022287.html
I see that now, I thought it was a typo since my compiler threw a
warning about it. If there are valid space savings for it, we need
to find a way to suppress the warning.
If you want to make the string const, put the const before the *.
Also, when we have to typecast our way around this crap (as in
skip_whitespace() in libbb where half the users typecast the return value
back to a normal char * when they use it, but it _is_ a normal char * to
begin with...)
I'm not a fan of const. Really.
I think that the compiler could care a flip about whether you are a
fanatic of const.
The words "fan" and "fanatic" have diverged rather a lot in the last century.
And "terrific" no longer means "inspiring terror". "Spectacular" is still
related to drawing spectators, but has other connotations as well.
Just FYI.
Mr. Landley, you are incredibly pugnacious. It makes for humorous
reading.

E
--
Erik Hovland
mail: erik at hovland.org
web: http://hovland.org/
PGP/GPG public key available on request
Jason Schoon
2006-06-18 17:45:13 UTC
Permalink
Post by Rob Landley
Yeah. Avoid using const.
I just threw "#define const" at the top of libbb.h, and fixed up half a dozen
files that broke when I did that (because they didn't #include libbb.h at the
top), and the result was actually 1500 bytes _smaller_. By not having
anything say const anywhere. (The resulting binary was about 2k smaller as
well.)
Just FYI. With gcc 4.0.3.
E
Rob
--
Never bet against the cheap plastic solution.
_______________________________________________
busybox mailing list
busybox at busybox.net
http://busybox.net/cgi-bin/mailman/listinfo/busybox
Running a very similar test with gcc 3.3.4 on x86 (Built clean using
defconfig, then added #define const to the top of libbb.h, then rebuilt
defconfig clean once more, fixing up the few places where header ordering
had caused an error), I actually ended up with a binary that was 14k
smaller.

If const were actually an enforced type in a strongly-typed language, I
might have concern about removing it. However, given that anybody can
simply ignore the const by casting, and that it appears to increase binary
size, not to mention causing unnecessary code fixups, I say whack it.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.busybox.net/pipermail/busybox/attachments/20060618/4e945334/attachment-0001.htm
Rob Landley
2006-06-18 16:24:16 UTC
Permalink
Post by Erik Hovland
Post by Rob Landley
Post by Erik Hovland
Post by Jason Schoon
Post by Erik Hovland
See my post and patch to utils.c and utils.h where I did this for
http://busybox.net/lists/busybox/2006-June/022287.html
I see that now, I thought it was a typo since my compiler threw a
warning about it. If there are valid space savings for it, we need
to find a way to suppress the warning.
If you want to make the string const, put the const before the *.
Also, when we have to typecast our way around this crap (as in
skip_whitespace() in libbb where half the users typecast the return value
back to a normal char * when they use it, but it _is_ a normal char * to
begin with...)
I'm not a fan of const. Really.
I think that the compiler could care a flip about whether you are a
fanatic of const.
The words "fan" and "fanatic" have diverged rather a lot in the last century.
And "terrific" no longer means "inspiring terror". "Spectacular" is still
related to drawing spectators, but has other connotations as well.

Just FYI.
Post by Erik Hovland
I could really care less about const either. I think
you probably agree that there is only one thing that will decide whether
const should be used or not, size of binary.
On which compiler version? If it's an optimization for gcc 2.95 but has no
effect on gcc 4.1, which is likely to be still relevant in 3 years?
Post by Erik Hovland
But if one is going to use const, they should do it right and at least
understand what the placement of const regarding the * operator does to
the string.
One reason I dislike is it having to understand (and maintain) non-obvious
rules like that.
Post by Erik Hovland
I have submitted two patches in so many weeks that
corrected const.
Needing to do this strikes me as a bad thing. Do current compilers benefit
from this, or just old ones? And by how much? This never came up in the
discussion, that I'm aware of...
Post by Erik Hovland
Both came after commits that put const in the wrong
spot and generated warnings. I think we all could do without those sorts
of mistakes so we can get to doing the work we want to do.
Yeah. Avoid using const.

I just threw "#define const" at the top of libbb.h, and fixed up half a dozen
files that broke when I did that (because they didn't #include libbb.h at the
top), and the result was actually 1500 bytes _smaller_. By not having
anything say const anywhere. (The resulting binary was about 2k smaller as
well.)

Just FYI. With gcc 4.0.3.
Post by Erik Hovland
E
Rob
--
Never bet against the cheap plastic solution.
Erik Hovland
2006-06-18 01:07:47 UTC
Permalink
Post by Rob Landley
Post by Erik Hovland
Post by Jason Schoon
Post by Erik Hovland
See my post and patch to utils.c and utils.h where I did this for
http://busybox.net/lists/busybox/2006-June/022287.html
I see that now, I thought it was a typo since my compiler threw a warning
about it. If there are valid space savings for it, we need to find a way
to suppress the warning.
If you want to make the string const, put the const before the *.
Also, when we have to typecast our way around this crap (as in
skip_whitespace() in libbb where half the users typecast the return value
back to a normal char * when they use it, but it _is_ a normal char * to
begin with...)
I'm not a fan of const. Really.
I think that the compiler could care a flip about whether you are a
fanatic of const. I could really care less about const either. I think
you probably agree that there is only one thing that will decide whether
const should be used or not, size of binary.

But if one is going to use const, they should do it right and at least
understand what the placement of const regarding the * operator does to
the string. I have submitted two patches in so many weeks that
corrected const. Both came after commits that put const in the wrong
spot and generated warnings. I think we all could do without those sorts
of mistakes so we can get to doing the work we want to do.

E
--
Erik Hovland
mail: erik at hovland.org
web: http://hovland.org/
PGP/GPG public key available on request
Rob Landley
2006-06-17 23:55:16 UTC
Permalink
Post by Erik Hovland
Post by Jason Schoon
Post by Jason Schoon
--- busybox/networking/tftp.c 2006-06-13 09:26:45.000000000 -0500
+++ busybox/networking/tftp.c.mine 2006-06-13
return blocksize;
}
-static char *tftp_option_get(char *buf, int len, const char const
*option)
+static char *tftp_option_get(char *buf, int len, char const *option)
static char *tftp_option_get(char *buf, int len, const char *const option)
The const in the right place would allow the compiler to further
optimize the case where both the dereferenced string and the pointer to
the string are not changed in the function that they are being passed
to. Not sure if that adds to a real space savings.
See my post and patch to utils.c and utils.h where I did this for
http://busybox.net/lists/busybox/2006-June/022287.html
I see that now, I thought it was a typo since my compiler threw a warning
about it. If there are valid space savings for it, we need to find a way
to suppress the warning.
If you want to make the string const, put the const before the *.
In build-at-once mode the compiler _really_ should be able to figure this
out...

Rob
--
Never bet against the cheap plastic solution.
Rob Landley
2006-06-17 23:58:00 UTC
Permalink
Post by Erik Hovland
Post by Jason Schoon
Post by Erik Hovland
See my post and patch to utils.c and utils.h where I did this for
http://busybox.net/lists/busybox/2006-June/022287.html
I see that now, I thought it was a typo since my compiler threw a warning
about it. If there are valid space savings for it, we need to find a way
to suppress the warning.
If you want to make the string const, put the const before the *.
Also, when we have to typecast our way around this crap (as in
skip_whitespace() in libbb where half the users typecast the return value
back to a normal char * when they use it, but it _is_ a normal char * to
begin with...)

I'm not a fan of const. Really.

Rob
--
Never bet against the cheap plastic solution.
Erik Hovland
2006-06-14 03:19:41 UTC
Permalink
Post by Jason Schoon
Post by Jason Schoon
--- busybox/networking/tftp.c 2006-06-13 09:26:45.000000000 -0500
+++ busybox/networking/tftp.c.mine 2006-06-13 09:26:11.000000000-0500
@@ -95,7 +95,7 @@
return blocksize;
}
-static char *tftp_option_get(char *buf, int len, const char const
*option)
+static char *tftp_option_get(char *buf, int len, char const *option)
static char *tftp_option_get(char *buf, int len, const char *const option)
The const in the right place would allow the compiler to further
optimize the case where both the dereferenced string and the pointer to
the string are not changed in the function that they are being passed
to. Not sure if that adds to a real space savings.
See my post and patch to utils.c and utils.h where I did this for
http://busybox.net/lists/busybox/2006-June/022287.html
I see that now, I thought it was a typo since my compiler threw a warning
about it. If there are valid space savings for it, we need to find a way to
suppress the warning.
If you want to make the string const, put the const before the *.

If you want to make the pointer that points to the string const, put the
const after the *.

const char* foo = bb_xstrdup(bar); // string (data) is const
char* const foo = bb_xstrdup(bar); // pointer to string is const

const char *const foo = bb_xstrdup(bar); // both string and pointer are
// const.

const char const* foo = bb_xstrdup(bar); // Illegal in C99
// warning with older gcc
// error with newer gcc

E
--
Erik Hovland
mail: erik at hovland.org
web: http://hovland.org/
PGP/GPG public key available on request
Bernhard Fischer
2006-06-14 15:31:03 UTC
Permalink
Post by Erik Hovland
--- busybox/networking/tftp.c 2006-06-13 09:26:45.000000000 -0500
+++ busybox/networking/tftp.c.mine 2006-06-13 09:26:11.000000000 -0500
@@ -95,7 +95,7 @@
return blocksize;
}
-static char *tftp_option_get(char *buf, int len, const char const *option)
+static char *tftp_option_get(char *buf, int len, char const *option)
static char *tftp_option_get(char *buf, int len, const char *const option)
The const in the right place would allow the compiler to further
optimize the case where both the dereferenced string and the pointer to
the string are not changed in the function that they are being passed
to. Not sure if that adds to a real space savings.
See my post and patch to utils.c and utils.h where I did this for
http://busybox.net/lists/busybox/2006-June/022287.html
Applied.
Typo of mine, sorry for any inconvenience this may have caused.
cheers,
Jason Schoon
2006-06-13 16:55:27 UTC
Permalink
Post by Jason Schoon
--- busybox/networking/tftp.c 2006-06-13 09:26:45.000000000 -0500
+++ busybox/networking/tftp.c.mine 2006-06-13 09:26:11.000000000-0500
@@ -95,7 +95,7 @@
return blocksize;
}
-static char *tftp_option_get(char *buf, int len, const char const
*option)
+static char *tftp_option_get(char *buf, int len, char const *option)
static char *tftp_option_get(char *buf, int len, const char *const option)
The const in the right place would allow the compiler to further
optimize the case where both the dereferenced string and the pointer to
the string are not changed in the function that they are being passed
to. Not sure if that adds to a real space savings.
See my post and patch to utils.c and utils.h where I did this for
http://busybox.net/lists/busybox/2006-June/022287.html
I see that now, I thought it was a typo since my compiler threw a warning
about it. If there are valid space savings for it, we need to find a way to
suppress the warning.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.busybox.net/pipermail/busybox/attachments/20060613/742ff7d5/attachment-0001.htm
Bernhard Fischer
2006-06-14 17:30:58 UTC
Permalink
Post by Erik Hovland
--- busybox/networking/tftp.c 2006-06-13 09:26:45.000000000 -0500
+++ busybox/networking/tftp.c.mine 2006-06-13 09:26:11.000000000 -0500
@@ -95,7 +95,7 @@
return blocksize;
}
-static char *tftp_option_get(char *buf, int len, const char const *option)
+static char *tftp_option_get(char *buf, int len, char const *option)
static char *tftp_option_get(char *buf, int len, const char *const option)
The const in the right place would allow the compiler to further
optimize the case where both the dereferenced string and the pointer to
the string are not changed in the function that they are being passed
to. Not sure if that adds to a real space savings.
See my post and patch to utils.c and utils.h where I did this for
http://busybox.net/lists/busybox/2006-June/022287.html
Applied.
Typo of mine, sorry for any inconvenience this may have caused.
cheers,
Bernhard Fischer
2006-06-14 15:30:14 UTC
Permalink
Post by Jason Schoon
Attached is one more patch for this to fix up two minor issues. A
duplicate const declaration, and my error in not checking if we were
using stdin or stdout before freeing the file handle.
Applied.
Also expanded the previous defines, to please Rob :)
Thanks,
Bernhard
Erik Hovland
2006-06-13 15:30:52 UTC
Permalink
--- busybox/networking/tftp.c 2006-06-13 09:26:45.000000000 -0500
+++ busybox/networking/tftp.c.mine 2006-06-13 09:26:11.000000000 -0500
@@ -95,7 +95,7 @@
return blocksize;
}
-static char *tftp_option_get(char *buf, int len, const char const *option)
+static char *tftp_option_get(char *buf, int len, char const *option)
You could write this as:
static char *tftp_option_get(char *buf, int len, const char *const option)

The const in the right place would allow the compiler to further
optimize the case where both the dereferenced string and the pointer to
the string are not changed in the function that they are being passed
to. Not sure if that adds to a real space savings.

See my post and patch to utils.c and utils.h where I did this for
another function:
http://busybox.net/lists/busybox/2006-June/022287.html

E
--
Erik Hovland
mail: erik at hovland.org
web: http://hovland.org/
PGP/GPG public key available on request
Bernhard Fischer
2006-06-14 17:30:05 UTC
Permalink
Post by Jason Schoon
Attached is one more patch for this to fix up two minor issues. A
duplicate const declaration, and my error in not checking if we were
using stdin or stdout before freeing the file handle.
Applied.
Also expanded the previous defines, to please Rob :)
Thanks,
Bernhard
Jason Schoon
2006-06-13 14:28:47 UTC
Permalink
Post by Bernhard Fischer
Post by Jason Schoon
Attached is one more patch for this to fix up two minor issues. A
duplicate const declaration, and my error in not checking if we were
using stdin or stdout before freeing the file handle.
--- busybox/networking/tftp.c.orig 2006-06-13 02:12:44.000000000 -0500
+++ busybox/networking/tftp.c 2006-06-13 02:13:55.000000000 -0500
@@ -95,7 +95,7 @@
return blocksize;
}
-static char *tftp_option_get(char *buf, int len, const char const *option)
->+static char *tftp_option_get(char *buf, int len, char const *option)
+static char *tftp_option_get(char *buf, int len, const char * const option)
Post by Jason Schoon
{
int opt_val = 0;
int opt_found = 0;
@@ -573,12 +573,12 @@
result = tftp(cmd, host, remotefile, fd, port, blocksize);
-#ifdef CONFIG_FEATURE_CLEAN_UP
if (!(fd == STDOUT_FILENO || fd == STDIN_FILENO)) {
+#ifdef CONFIG_FEATURE_CLEAN_UP
Do i understand you right?
if (ENABLE_FEATURE_CLEAN_UP) {
if (!(fd == STDOUT_FILENO || fd == STDIN_FILENO))
close(fd);
if (cmd == tftp_cmd_get && result != EXIT_SUCCESS)
unlink(localfile);
}
Why do you want to bypass feature_cleanup for rm'ing the failed file?
Just curious..
I'll admit it was late when I made those changes, but I'm confused
now. The feature_cleanup stuff shouldn't have changed, I just moved
the check for stdin/stdout outside of that ifdef because we want to
unlink the file in any error case, regardless of feature_cleanup being
enabled. Moving the check outside of feature_cleanup saves checking
the file descriptors twice.

Checking....Yeah, it appears to do what I want. Is there something
else I am missing?

When I grabbed the patch back off the mailing list (no local copy
here), it didn't seem to apply completely cleanly. It possible either
my browser or something else horked it, but attached is one that I
just verified patches cleanly, just in case.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox_tftp.patch
Type: text/x-patch
Size: 777 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20060613/f569e4cc/attachment.bin
Bernhard Fischer
2006-06-13 12:26:21 UTC
Permalink
Post by Jason Schoon
Attached is one more patch for this to fix up two minor issues. A
duplicate const declaration, and my error in not checking if we were
using stdin or stdout before freeing the file handle.
--- busybox/networking/tftp.c.orig 2006-06-13 02:12:44.000000000 -0500
+++ busybox/networking/tftp.c 2006-06-13 02:13:55.000000000 -0500
@@ -95,7 +95,7 @@
return blocksize;
}
-static char *tftp_option_get(char *buf, int len, const char const *option)
->+static char *tftp_option_get(char *buf, int len, char const *option)
+static char *tftp_option_get(char *buf, int len, const char * const option)
Post by Jason Schoon
{
int opt_val = 0;
int opt_found = 0;
@@ -573,12 +573,12 @@
result = tftp(cmd, host, remotefile, fd, port, blocksize);
-#ifdef CONFIG_FEATURE_CLEAN_UP
if (!(fd == STDOUT_FILENO || fd == STDIN_FILENO)) {
+#ifdef CONFIG_FEATURE_CLEAN_UP
Do i understand you right?
if (ENABLE_FEATURE_CLEAN_UP) {
if (!(fd == STDOUT_FILENO || fd == STDIN_FILENO))
close(fd);
if (cmd == tftp_cmd_get && result != EXIT_SUCCESS)
unlink(localfile);
}
Why do you want to bypass feature_cleanup for rm'ing the failed file?
Just curious..
Jason Schoon
2006-06-10 16:59:57 UTC
Permalink
Post by Bernhard Fischer
Applied as http://busybox.net/cgi-bin/viewcvs.cgi?rev=15355&view=rev I
think I didn't damaged it too much..
Thanks to anyone who was involved.
cheers,
Bernhard
Looks good, I'll have to give a try when I am back in front of my system. I
plan to take a look at the IPv6 patch and massage it a bit. Telnet client,
remote syslog, ftpputget are also on my list of planned conversions to IPv6.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.busybox.net/pipermail/busybox/attachments/20060610/c917621d/attachment.htm
Jason Schoon
2006-06-13 07:20:54 UTC
Permalink
Post by Bernhard Fischer
Post by Bernhard Fischer
Post by Jason Schoon
Post by Bernhard Fischer
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).
- 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..
Here is an updated patch. First, I realized I had my file cleanup code in
the wrong place. It should be called unconditionally, not in
FEATURE_CLEANUP. I then went through and cleaned up some more of the code a
bit, removing useless comments and magic numbers as best I could. I did not
yet try to shrink tftp() or switch to the ENABLE_ macros.
I'm currently looking at opportunities to shrink it a bit (on top of our
cummulative 04 patch).
Applied as http://busybox.net/cgi-bin/viewcvs.cgi?rev=15355&view=rev I
think I didn't damaged it too much..
Thanks to anyone who was involved.
cheers,
Bernhard
Attached is one more patch for this to fix up two minor issues. A
duplicate const declaration, and my error in not checking if we were
using stdin or stdout before freeing the file handle.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox_tftp.patch
Type: text/x-patch
Size: 777 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20060613/0421c7d1/attachment.bin
Bernhard Fischer
2006-06-10 14:21:16 UTC
Permalink
Post by Bernhard Fischer
Post by Jason Schoon
Post by Bernhard Fischer
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).
- 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..
Here is an updated patch. First, I realized I had my file cleanup code in
the wrong place. It should be called unconditionally, not in
FEATURE_CLEANUP. I then went through and cleaned up some more of the code a
bit, removing useless comments and magic numbers as best I could. I did not
yet try to shrink tftp() or switch to the ENABLE_ macros.
I'm currently looking at opportunities to shrink it a bit (on top of our
cummulative 04 patch).
Applied as http://busybox.net/cgi-bin/viewcvs.cgi?rev=15355&view=rev I
think I didn't damaged it too much..
Thanks to anyone who was involved.
cheers,
Bernhard
Rob Landley
2006-06-13 16:07:42 UTC
Permalink
Post by Jason Schoon
Here is an updated patch. First, I realized I had my file cleanup code in
the wrong place. It should be called unconditionally, not in
FEATURE_CLEANUP. I then went through and cleaned up some more of the code
a bit, removing useless comments and magic numbers as best I could. I did
not yet try to shrink tftp() or switch to the ENABLE_ macros.
Catching up on the backlog:

+static const char MODE_OCTET[] = "octet";
+static unsigned char MODE_OCTET_LEN = sizeof(MODE_OCTET);

Statics (even const statics) actually wind up in the resulting binary.
#defines and enums do not (nor would just using sizeof() when you actually
need it.)

In theory we can tell gcc "-fno-keep-static-consts" and
"-fmerge-all-constants" in order to avoid being _stupid_ about this, but I
just tried adding them and the resulting binary was identical...

+/* RFC2348 says between 8 and 65464 */
+#define TFTP_OCTECTS_MIN 8
+#define TFTP_OCTECTS_MAX 65464
...
- (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;

Please keep the constants inline but put the comment about how they're from
RFC2348 where they're used. (What is it supposed to buy you, moving a
constant that should never change out of line?)

+#define tftp_cmd_get ENABLE_FEATURE_TFTP_GET

Is there a down side to just replacing the uses of it with the correct macro?

Ah, nevermind. I see Bernhard already applied it...

Rob
--
Never bet against the cheap plastic solution.
Bernhard Fischer
2006-06-14 15:07:54 UTC
Permalink
Post by Rob Landley
Post by Jason Schoon
Here is an updated patch. First, I realized I had my file cleanup code in
the wrong place. It should be called unconditionally, not in
FEATURE_CLEANUP. I then went through and cleaned up some more of the code
a bit, removing useless comments and magic numbers as best I could. I did
not yet try to shrink tftp() or switch to the ENABLE_ macros.
+static const char MODE_OCTET[] = "octet";
+static unsigned char MODE_OCTET_LEN = sizeof(MODE_OCTET);
Statics (even const statics) actually wind up in the resulting binary.
No.
It doesn't make any difference whatsoever if you
#define MODE_OCTET "octet"
or
static const char * const MODE_OCTET = "octet";
that i finally applied.
Post by Rob Landley
#defines and enums do not (nor would just using sizeof() when you actually
need it.)
In theory we can tell gcc "-fno-keep-static-consts" and
"-fmerge-all-constants" in order to avoid being _stupid_ about this, but I
just tried adding them and the resulting binary was identical...
+/* RFC2348 says between 8 and 65464 */
+#define TFTP_OCTECTS_MIN 8
+#define TFTP_OCTECTS_MAX 65464
...
- (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;
Please keep the constants inline but put the comment about how they're from
No idea what you mean with 'inline' there.
We don't want magic numbers. Just look at an arbitrary driver written by
a vendor. It's unreadable because they tend to use magic numbers, 'cause
they are soooo l33t.

out-of-line would have been
int get_RFC2348_max_octets_value_as_an_int(void)
{ return 0xffb8;/* 0177670; see RFC2348 as shipped with your netware
diskettes */}

#define TFTP_OCTECTS_MAX 65464
*is* inline.
Post by Rob Landley
RFC2348 where they're used. (What is it supposed to buy you, moving a
constant that should never change out of line?)
+#define tftp_cmd_get ENABLE_FEATURE_TFTP_GET
Is there a down side to just replacing the uses of it with the correct macro?
I was lazy and wanted to keep the diff small. I'll remember to to expand
them next time, promised.
Post by Rob Landley
Ah, nevermind. I see Bernhard already applied it...
Rob
--
Never bet against the cheap plastic solution.
Rob Landley
2006-06-14 17:49:01 UTC
Permalink
Post by Bernhard Fischer
Post by Rob Landley
Post by Jason Schoon
Here is an updated patch. First, I realized I had my file cleanup code
in the wrong place. It should be called unconditionally, not in
FEATURE_CLEANUP. I then went through and cleaned up some more of the
code a bit, removing useless comments and magic numbers as best I could.
I did not yet try to shrink tftp() or switch to the ENABLE_ macros.
+static const char MODE_OCTET[] = "octet";
+static unsigned char MODE_OCTET_LEN = sizeof(MODE_OCTET);
Statics (even const statics) actually wind up in the resulting binary.
No.
It doesn't make any difference whatsoever if you
#define MODE_OCTET "octet"
or
static const char * const MODE_OCTET = "octet";
that i finally applied.
So this has been fixed since:

http://busybox.net/lists/busybox/2005-July/015160.html

Was it an extra flag we added, or a bump in compiler version, or...?
Post by Bernhard Fischer
Post by Rob Landley
+/* RFC2348 says between 8 and 65464 */
+#define TFTP_OCTECTS_MIN 8
+#define TFTP_OCTECTS_MAX 65464
...
- (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;
Please keep the constants inline but put the comment about how they're from
No idea what you mean with 'inline' there.
We don't want magic numbers. Just look at an arbitrary driver written by
a vendor. It's unreadable because they tend to use magic numbers, 'cause
they are soooo l33t.
I mean that before this change the actual values being tested against were
right there where the test was. The fact the constants come from an RFC is a
good thing to note in a comment, but why move the actual values away from
where the test is?

Adding the comment is good, but why is the rest of this change an improvement?
The value's only used in one place, why on earth would you make a
one-instance #define that's not even on the screen at the same time as the
test?

(And what the heck is an OCTECT, anyway? Last I heard, "octet" has one C in
it. Plus when I see "OCTET" I think "8 bytes", since the word for 8 bits is
"BYTE". But that's well into nit-picking territory by now...)
Post by Bernhard Fischer
Post by Rob Landley
RFC2348 where they're used. (What is it supposed to buy you, moving a
constant that should never change out of line?)
+#define tftp_cmd_get ENABLE_FEATURE_TFTP_GET
Is there a down side to just replacing the uses of it with the correct macro?
I was lazy and wanted to keep the diff small. I'll remember to to expand
them next time, promised.
*shrug* We can always do more cleanups later. I just like to group 'em to
cut down on svn commit noise.

Rob
--
Never bet against the cheap plastic solution.
Jason Schoon
2006-06-14 18:12:41 UTC
Permalink
Post by Rob Landley
I mean that before this change the actual values being tested against were
right there where the test was. The fact the constants come from an RFC is a
good thing to note in a comment, but why move the actual values away from
where the test is?
Adding the comment is good, but why is the rest of this change an improvement?
The value's only used in one place, why on earth would you make a
one-instance #define that's not even on the screen at the same time as the
test?
I think this goes back to Bernhard's comment about magic numbers. I
100% concur with his assesment of magic numbers, and consider them to
be equivalent to phone solicitors on the evil ladder.
Post by Rob Landley
(And what the heck is an OCTECT, anyway? Last I heard, "octet" has one C in
it. Plus when I see "OCTET" I think "8 bytes", since the word for 8 bits is
"BYTE". But that's well into nit-picking territory by now...)
Check out the TFTP RFC. AFAIK, they coined the OCTECT term, and it is
definately not an octet or anything having to do with a grouping of 8.
Rob Landley
2006-06-14 19:00:24 UTC
Permalink
Post by Jason Schoon
Post by Rob Landley
I mean that before this change the actual values being tested against
were right there where the test was. The fact the constants come from an
RFC is a good thing to note in a comment, but why move the actual values
away from where the test is?
Adding the comment is good, but why is the rest of this change an
improvement? The value's only used in one place, why on earth would you
make a one-instance #define that's not even on the screen at the same
time as the test?
I think this goes back to Bernhard's comment about magic numbers. I
100% concur with his assesment of magic numbers, and consider them to
be equivalent to phone solicitors on the evil ladder.
They're in the spec. We must use these numbers. Documenting which spec says
we must use exactly these numbers is a good thing. But attempting to hide
that we ARE using these numbers (in exactly one place) by moving them up to
the top of the line and #defining them is _STUPID_. What, these numbers
acquire some kind of nobility by being separated far from the part of the
code that uses them? These numbers wouldn't dare be seen soiling themselves
in actual code, but must be hidden away behind some macro name to guard their
virtue?

Doing this adds several lines to the length of the program and breaks the
logical flow, it means that you have to stop what you're doing and look
something up in order to follow what's actually going on, and this happens
for no reason. Why is this a good thing?
Post by Jason Schoon
Post by Rob Landley
(And what the heck is an OCTECT, anyway? Last I heard, "octet" has one C
in it. Plus when I see "OCTET" I think "8 bytes", since the word for 8
bits is "BYTE". But that's well into nit-picking territory by now...)
Check out the TFTP RFC. AFAIK, they coined the OCTECT term, and it is
definately not an octet or anything having to do with a grouping of 8.
http://www.faqs.org/rfcs/rfc2348.html

Does not contain the string "octect". It does contain 17 occurrences of the
string "octet", which it uses as a synonym for byte and which they refer to
rfc1350 for where "octet" is defined as "raw 8 bit bytes", and the extended
description goes off on a tangent about the DEC-20 (a 36-bit machine).
Apparently, in 1992 there was still one of those in use somewhere.

I continue top pretty strongly believe that if a constant that doesn't change
is only ever used in one place, making a #define for that constant is the
wrong thing to do, and instead a comment should be placed at the site of the
one use of the thing specifying what the constant means and/or where it comes
from.

Rob
--
Never bet against the cheap plastic solution.
Jason Schoon
2006-06-14 19:44:18 UTC
Permalink
Post by Rob Landley
http://www.faqs.org/rfcs/rfc2348.html
Does not contain the string "octect". It does contain 17 occurrences of the
string "octet", which it uses as a synonym for byte and which they refer to
rfc1350 for where "octet" is defined as "raw 8 bit bytes", and the extended
description goes off on a tangent about the DEC-20 (a 36-bit machine).
Apparently, in 1992 there was still one of those in use somewhere.
Doh, you're right, I thought for sure I looked at that, having been
skeptical myself. Octect is a ASN.1 type used widely in SNMP. My mind is
apparently becoming cross-wired.

I continue top pretty strongly believe that if a constant that doesn't
Post by Rob Landley
change
is only ever used in one place, making a #define for that constant is the
wrong thing to do, and instead a comment should be placed at the site of the
one use of the thing specifying what the constant means and/or where it comes
from.
I don't necessarily care if it is a #define or not. Having a comment giving
the source of the magic number is probably sufficient also. I do think you
have 2 contradictary statements here though:

"They're in the spec. We must use these numbers."

vs.

"...it means that you have to stop what you're doing and look
something up in order to follow what's actually going on, and this happens
for no reason."
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://busybox.net/lists/busybox/attachments/20060614/41849778/attachment.html
Rob Landley
2006-06-14 20:03:52 UTC
Permalink
Post by Jason Schoon
I don't necessarily care if it is a #define or not. Having a comment
giving the source of the magic number is probably sufficient also. I do
"They're in the spec. We must use these numbers."
vs.
"...it means that you have to stop what you're doing and look
something up in order to follow what's actually going on, and this happens
for no reason."
What's contradictory? If you undo the recent damage, you have:

static int tftp_blocksize_check(int blocksize, int bufsize)
{
/* Check if the blocksize is valid:
* RFC2348 says between 8 and 65464,
* but our implementation makes it impossible
* to use blocksizes smaller than 22 octets.
*/

if ((bufsize && (blocksize > bufsize)) ||
(blocksize < 8) || (blocksize > 65564)) {
bb_error_msg("bad blocksize");
return 0;
}

return blocksize;
}

There's _already_ a comment there that has the constants in it (which is why
moving a second copy of those constants off the screen into a #define two
pages earlier in the file is just crazy). And that comment actually implies
that we should be checking for 22 at the low end, rather than 8, a problem
which is NOT noticeable when the #define is off on another page.

That's why doing that is bad.

Rob
--
Never bet against the cheap plastic solution.
Rob Landley
2006-06-14 22:03:51 UTC
Permalink
Post by Jason Schoon
I don't necessarily care if it is a #define or not. Having a comment
giving the source of the magic number is probably sufficient also. I do
"They're in the spec. We must use these numbers."
vs.
"...it means that you have to stop what you're doing and look
something up in order to follow what's actually going on, and this happens
for no reason."
What's contradictory? If you undo the recent damage, you have:

static int tftp_blocksize_check(int blocksize, int bufsize)
{
/* Check if the blocksize is valid:
* RFC2348 says between 8 and 65464,
* but our implementation makes it impossible
* to use blocksizes smaller than 22 octets.
*/

if ((bufsize && (blocksize > bufsize)) ||
(blocksize < 8) || (blocksize > 65564)) {
bb_error_msg("bad blocksize");
return 0;
}

return blocksize;
}

There's _already_ a comment there that has the constants in it (which is why
moving a second copy of those constants off the screen into a #define two
pages earlier in the file is just crazy). And that comment actually implies
that we should be checking for 22 at the low end, rather than 8, a problem
which is NOT noticeable when the #define is off on another page.

That's why doing that is bad.

Rob
--
Never bet against the cheap plastic solution.
Jason Schoon
2006-06-14 21:44:11 UTC
Permalink
Post by Rob Landley
http://www.faqs.org/rfcs/rfc2348.html
Does not contain the string "octect". It does contain 17 occurrences of the
string "octet", which it uses as a synonym for byte and which they refer to
rfc1350 for where "octet" is defined as "raw 8 bit bytes", and the extended
description goes off on a tangent about the DEC-20 (a 36-bit machine).
Apparently, in 1992 there was still one of those in use somewhere.
Doh, you're right, I thought for sure I looked at that, having been
skeptical myself. Octect is a ASN.1 type used widely in SNMP. My mind is
apparently becoming cross-wired.

I continue top pretty strongly believe that if a constant that doesn't
Post by Rob Landley
change
is only ever used in one place, making a #define for that constant is the
wrong thing to do, and instead a comment should be placed at the site of the
one use of the thing specifying what the constant means and/or where it comes
from.
I don't necessarily care if it is a #define or not. Having a comment giving
the source of the magic number is probably sufficient also. I do think you
have 2 contradictary statements here though:

"They're in the spec. We must use these numbers."

vs.

"...it means that you have to stop what you're doing and look
something up in order to follow what's actually going on, and this happens
for no reason."
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.busybox.net/pipermail/busybox/attachments/20060614/41849778/attachment-0001.htm
Rob Landley
2006-06-14 21:00:26 UTC
Permalink
Post by Jason Schoon
Post by Rob Landley
I mean that before this change the actual values being tested against
were right there where the test was. The fact the constants come from an
RFC is a good thing to note in a comment, but why move the actual values
away from where the test is?
Adding the comment is good, but why is the rest of this change an
improvement? The value's only used in one place, why on earth would you
make a one-instance #define that's not even on the screen at the same
time as the test?
I think this goes back to Bernhard's comment about magic numbers. I
100% concur with his assesment of magic numbers, and consider them to
be equivalent to phone solicitors on the evil ladder.
They're in the spec. We must use these numbers. Documenting which spec says
we must use exactly these numbers is a good thing. But attempting to hide
that we ARE using these numbers (in exactly one place) by moving them up to
the top of the line and #defining them is _STUPID_. What, these numbers
acquire some kind of nobility by being separated far from the part of the
code that uses them? These numbers wouldn't dare be seen soiling themselves
in actual code, but must be hidden away behind some macro name to guard their
virtue?

Doing this adds several lines to the length of the program and breaks the
logical flow, it means that you have to stop what you're doing and look
something up in order to follow what's actually going on, and this happens
for no reason. Why is this a good thing?
Post by Jason Schoon
Post by Rob Landley
(And what the heck is an OCTECT, anyway? Last I heard, "octet" has one C
in it. Plus when I see "OCTET" I think "8 bytes", since the word for 8
bits is "BYTE". But that's well into nit-picking territory by now...)
Check out the TFTP RFC. AFAIK, they coined the OCTECT term, and it is
definately not an octet or anything having to do with a grouping of 8.
http://www.faqs.org/rfcs/rfc2348.html

Does not contain the string "octect". It does contain 17 occurrences of the
string "octet", which it uses as a synonym for byte and which they refer to
rfc1350 for where "octet" is defined as "raw 8 bit bytes", and the extended
description goes off on a tangent about the DEC-20 (a 36-bit machine).
Apparently, in 1992 there was still one of those in use somewhere.

I continue top pretty strongly believe that if a constant that doesn't change
is only ever used in one place, making a #define for that constant is the
wrong thing to do, and instead a comment should be placed at the site of the
one use of the thing specifying what the constant means and/or where it comes
from.

Rob
--
Never bet against the cheap plastic solution.
Jason Schoon
2006-06-14 20:12:37 UTC
Permalink
Post by Rob Landley
I mean that before this change the actual values being tested against were
right there where the test was. The fact the constants come from an RFC is a
good thing to note in a comment, but why move the actual values away from
where the test is?
Adding the comment is good, but why is the rest of this change an improvement?
The value's only used in one place, why on earth would you make a
one-instance #define that's not even on the screen at the same time as the
test?
I think this goes back to Bernhard's comment about magic numbers. I
100% concur with his assesment of magic numbers, and consider them to
be equivalent to phone solicitors on the evil ladder.
Post by Rob Landley
(And what the heck is an OCTECT, anyway? Last I heard, "octet" has one C in
it. Plus when I see "OCTET" I think "8 bytes", since the word for 8 bits is
"BYTE". But that's well into nit-picking territory by now...)
Check out the TFTP RFC. AFAIK, they coined the OCTECT term, and it is
definately not an octet or anything having to do with a grouping of 8.
Rob Landley
2006-06-14 19:49:05 UTC
Permalink
Post by Bernhard Fischer
Post by Rob Landley
Post by Jason Schoon
Here is an updated patch. First, I realized I had my file cleanup code
in the wrong place. It should be called unconditionally, not in
FEATURE_CLEANUP. I then went through and cleaned up some more of the
code a bit, removing useless comments and magic numbers as best I could.
I did not yet try to shrink tftp() or switch to the ENABLE_ macros.
+static const char MODE_OCTET[] = "octet";
+static unsigned char MODE_OCTET_LEN = sizeof(MODE_OCTET);
Statics (even const statics) actually wind up in the resulting binary.
No.
It doesn't make any difference whatsoever if you
#define MODE_OCTET "octet"
or
static const char * const MODE_OCTET = "octet";
that i finally applied.
So this has been fixed since:

http://busybox.net/lists/busybox/2005-July/015160.html

Was it an extra flag we added, or a bump in compiler version, or...?
Post by Bernhard Fischer
Post by Rob Landley
+/* RFC2348 says between 8 and 65464 */
+#define TFTP_OCTECTS_MIN 8
+#define TFTP_OCTECTS_MAX 65464
...
- (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;
Please keep the constants inline but put the comment about how they're from
No idea what you mean with 'inline' there.
We don't want magic numbers. Just look at an arbitrary driver written by
a vendor. It's unreadable because they tend to use magic numbers, 'cause
they are soooo l33t.
I mean that before this change the actual values being tested against were
right there where the test was. The fact the constants come from an RFC is a
good thing to note in a comment, but why move the actual values away from
where the test is?

Adding the comment is good, but why is the rest of this change an improvement?
The value's only used in one place, why on earth would you make a
one-instance #define that's not even on the screen at the same time as the
test?

(And what the heck is an OCTECT, anyway? Last I heard, "octet" has one C in
it. Plus when I see "OCTET" I think "8 bytes", since the word for 8 bits is
"BYTE". But that's well into nit-picking territory by now...)
Post by Bernhard Fischer
Post by Rob Landley
RFC2348 where they're used. (What is it supposed to buy you, moving a
constant that should never change out of line?)
+#define tftp_cmd_get ENABLE_FEATURE_TFTP_GET
Is there a down side to just replacing the uses of it with the correct macro?
I was lazy and wanted to keep the diff small. I'll remember to to expand
them next time, promised.
*shrug* We can always do more cleanups later. I just like to group 'em to
cut down on svn commit noise.

Rob
--
Never bet against the cheap plastic solution.
Bernhard Fischer
2006-06-14 17:07:48 UTC
Permalink
Post by Rob Landley
Post by Jason Schoon
Here is an updated patch. First, I realized I had my file cleanup code in
the wrong place. It should be called unconditionally, not in
FEATURE_CLEANUP. I then went through and cleaned up some more of the code
a bit, removing useless comments and magic numbers as best I could. I did
not yet try to shrink tftp() or switch to the ENABLE_ macros.
+static const char MODE_OCTET[] = "octet";
+static unsigned char MODE_OCTET_LEN = sizeof(MODE_OCTET);
Statics (even const statics) actually wind up in the resulting binary.
No.
It doesn't make any difference whatsoever if you
#define MODE_OCTET "octet"
or
static const char * const MODE_OCTET = "octet";
that i finally applied.
Post by Rob Landley
#defines and enums do not (nor would just using sizeof() when you actually
need it.)
In theory we can tell gcc "-fno-keep-static-consts" and
"-fmerge-all-constants" in order to avoid being _stupid_ about this, but I
just tried adding them and the resulting binary was identical...
+/* RFC2348 says between 8 and 65464 */
+#define TFTP_OCTECTS_MIN 8
+#define TFTP_OCTECTS_MAX 65464
...
- (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;
Please keep the constants inline but put the comment about how they're from
No idea what you mean with 'inline' there.
We don't want magic numbers. Just look at an arbitrary driver written by
a vendor. It's unreadable because they tend to use magic numbers, 'cause
they are soooo l33t.

out-of-line would have been
int get_RFC2348_max_octets_value_as_an_int(void)
{ return 0xffb8;/* 0177670; see RFC2348 as shipped with your netware
diskettes */}

#define TFTP_OCTECTS_MAX 65464
*is* inline.
Post by Rob Landley
RFC2348 where they're used. (What is it supposed to buy you, moving a
constant that should never change out of line?)
+#define tftp_cmd_get ENABLE_FEATURE_TFTP_GET
Is there a down side to just replacing the uses of it with the correct macro?
I was lazy and wanted to keep the diff small. I'll remember to to expand
them next time, promised.
Post by Rob Landley
Ah, nevermind. I see Bernhard already applied it...
Rob
--
Never bet against the cheap plastic solution.
Bernhard Fischer
2006-06-10 13:24:10 UTC
Permalink
Post by Jason Schoon
Post by Bernhard Fischer
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).
- 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..
Here is an updated patch. First, I realized I had my file cleanup code in
the wrong place. It should be called unconditionally, not in
FEATURE_CLEANUP. I then went through and cleaned up some more of the code a
bit, removing useless comments and magic numbers as best I could. I did not
yet try to shrink tftp() or switch to the ENABLE_ macros.
I'm currently looking at opportunities to shrink it a bit (on top of our
cummulative 04 patch).
Post by Jason Schoon
--- busybox/networking/tftp.c 2006-04-24 15:22:35.000000000 -0500
+++ busybox/networking/tftp.c.mine 2006-04-24 15:22:05.000000000 -0500
@@ -34,13 +34,22 @@
+static const char OPTION_BLOCKSIZE[] = "blocksize";
+static unsigned char OPTION_BLOCKSIZE_LEN = sizeof(OPTION_BLOCKSIZE);
- memcpy(cp, "blksize", 8);
- cp += 8;
-
+ memcpy(cp, OPTION_BLOCKSIZE, OPTION_BLOCKSIZE_LEN);
+ cp += OPTION_BLOCKSIZE_LEN;
cp += snprintf(cp, 6, "%d", len) + 1;
That typo i managed to introduce in the 03 patch will of course not be
in the final patch ("blocksize" != "blksize").

Did anyone look at that IPv6-support patch for tftp in the bugtracker?



Also there is a patch which fixes a bug on bad packets
(http://bugs.busybox.net/view.php?id=634). The proposed patch looks like
it overdoes it a bit. Perhaps it would be better to just reset the bad
packet completely before continuing, have to look if that is feasable.
Thoughts?
Rob Landley
2006-06-13 18:07:43 UTC
Permalink
Post by Jason Schoon
Here is an updated patch. First, I realized I had my file cleanup code in
the wrong place. It should be called unconditionally, not in
FEATURE_CLEANUP. I then went through and cleaned up some more of the code
a bit, removing useless comments and magic numbers as best I could. I did
not yet try to shrink tftp() or switch to the ENABLE_ macros.
Catching up on the backlog:

+static const char MODE_OCTET[] = "octet";
+static unsigned char MODE_OCTET_LEN = sizeof(MODE_OCTET);

Statics (even const statics) actually wind up in the resulting binary.
#defines and enums do not (nor would just using sizeof() when you actually
need it.)

In theory we can tell gcc "-fno-keep-static-consts" and
"-fmerge-all-constants" in order to avoid being _stupid_ about this, but I
just tried adding them and the resulting binary was identical...

+/* RFC2348 says between 8 and 65464 */
+#define TFTP_OCTECTS_MIN 8
+#define TFTP_OCTECTS_MAX 65464
...
- (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;

Please keep the constants inline but put the comment about how they're from
RFC2348 where they're used. (What is it supposed to buy you, moving a
constant that should never change out of line?)

+#define tftp_cmd_get ENABLE_FEATURE_TFTP_GET

Is there a down side to just replacing the uses of it with the correct macro?

Ah, nevermind. I see Bernhard already applied it...

Rob
--
Never bet against the cheap plastic solution.
Jason Schoon
2006-04-24 20:26:48 UTC
Permalink
Post by Bernhard Fischer
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).
- 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..
Here is an updated patch. First, I realized I had my file cleanup code in
the wrong place. It should be called unconditionally, not in
FEATURE_CLEANUP. I then went through and cleaned up some more of the code a
bit, removing useless comments and magic numbers as best I could. I did not
yet try to shrink tftp() or switch to the ENABLE_ macros.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.busybox.net/pipermail/busybox/attachments/20060424/fe28a2d8/attachment-0002.htm
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox.tftp_touchup.04.patch
Type: text/x-patch
Size: 13266 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20060424/fe28a2d8/attachment.bin
Bernhard Fischer
2006-04-22 11:13:58 UTC
Permalink
Post by KRONSTORFER Horst
hi!
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
/* 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);
}
KRONSTORFER Horst
2006-04-21 15:04:34 UTC
Permalink
-----Original Message-----
From: Jason Schoon [mailto:floydpink at gmail.com]
Sent: Fri 4/21/2006 4:46 PM
To: KRONSTORFER Horst
Cc: busybox at busybox.net
Subject: Re: [PATCH] Fix 2 possible SEGVs in tftp client
Post by KRONSTORFER Horst
hi!
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
/* 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.busybox.net/pipermail/busybox/attachments/20060421/57d59dad/attachment-0002.htm
KRONSTORFER Horst
2006-04-21 14:04:10 UTC
Permalink
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.

-h





-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.busybox.net/pipermail/busybox/attachments/20060421/8a027a4c/attachment-0002.htm
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tftp.patch
Type: application/octet-stream
Size: 793 bytes
Desc: tftp.patch
Url : http://lists.busybox.net/pipermail/busybox/attachments/20060421/8a027a4c/attachment.obj
Jason Schoon
2006-04-21 14:46:13 UTC
Permalink
Post by KRONSTORFER Horst
hi!
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! */

(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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.busybox.net/pipermail/busybox/attachments/20060421/a5c89983/attachment-0002.htm
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox_tftp_remove_file.patch
Type: text/x-patch
Size: 513 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20060421/a5c89983/attachment.bin
Loading...