Re: [PATCH] Cut down buffer size in ftp_anon_connect()

[Kris Katterjohn <kjak () ispwest com>]

Matthew Murphy wrote:
> Kris Katterjohn wrote:
> > > The attached patch cuts down the size of the 'command' buffer in
> > > ftp_anon-connect() from 512 to 270. ftp->user can hold 64 bytes and ftp->pass
> > > can hold 256, so 270 will hold "PASS [ftp->pass]\r\n" with a few extra bytes
> > > in there. It also uses sizeof in snprintf() instead of just a number.
> > >
> > > Thanks,
> > > Kris Katterjohn
>
> I have two potential concerns about this patch.  First of all, I'd
> recommend applying it with parenthesis around sizeof expressions, as in:
>
>     sizeof(command) - 1
>
> rather than:
>
>     sizeof command - 1
>
> Most compilers are tolerant of both forms, but I'm more used to seeing
> the latter, and it is (IMO) cleaner.

I don't know what you mean by "tolerant" of both forms. sizeof only requires
the parentheses when it's taking the size of a data type:

sizeof(int)

Parentheses aren't required when it's a variable (like in the patch). Not that
I have a problem with the parentheses in this situation, just clearing things
up :)

> Further, in looking at your choice of buffer size, I'm wondering why 270
> instead of 264?
>
> PASS [256 chars]\r\n\0
>
> is exactly 256+8 = 264.  Choosing 264 also makes the buffer eight-byte
> aligned, which may save a few bytes of stack space (in addition to the
> six byte slack) or make the binary a (microscopic) hair faster by
> avoiding alignment issues.

No real reason, just that when the size of the user/pass buffers are in
another file (and not RIGHT there to be seen) and the string *could* possibly
change (not that I foresee that anytime soon), I sometimes give it a few extra
bytes.

264 sounds okay to me, though.


Thanks,
Kris


_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev