[PATCH] Fix 2 possible SEGVs in tftp client

[Rob Landley]

On Wednesday 14 June 2006 1:07 pm, Bernhard Fischer wrote:
> On Tue, Jun 13, 2006 at 02:07:43PM -0400, Rob Landley wrote:
> >On Monday 24 April 2006 4:26 pm, Jason Schoon wrote:
> >> 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.
>
> 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...?

> >+/* 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...)

> >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.