[PATCH] Fix 2 possible SEGVs in tftp client

[Rob Landley]

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