Ethereal-dev: Re: [Ethereal-dev] What does the "maxlength" argument to "tvb_get_nstringz*" mea
On Mon, 28 Apr 2003, Guy Harris wrote:
> The question is whether the "maxlength" argument to those routines
> should be
>
> 1) the maximum number of non-'\0' characters copied
>
> or
>
> 2) the size of the buffer into which the string is being copied.
>
> At least some code appears to assume it's 1), e.g. the FTP, iSCSI and
> MGCP dissectors; some other code appears to assume it's 2), e.g., I
> suspect, the AIM dissector (although I'm not sure of that - it depends
> on what the buddy name length is; the buffer is MAX_BUDDYNAME_LENGTH
> bytes long).
>
> If it's supposed to be 1), the change should be backed out and all the
> code that assumes it's 2) needs to be fixed (typically by increasing the
> size of the buffer by 1, to leave room for the trailing '\0').
>
> If it's supposed to be 2), "tvb_get_nstringz0()" needs to be changed to
> set "buffer[maxlength-1]" to '\0' rather than setting
> "buffer[maxlength]" to '\0', and the code that assumes it's 1) needs to
> be fixed (by passing the size of the buffer, rather than the size of the
> buffer - 1, as the maximum length argument).
I'm leaning towards 2), since it errs on the side of safety. If 1) is
implemented the caller assumes 2), you have a potential buffer overflow.
Conversely, if 2) is implemented and 1) is assumed you waste a byte of
memory. According to Timo's original message, about half the dissectors
that call tvb_get_nstringz*() assume 2). Here's his list:
packet-smb-browse.c:676
packet-rsync.c:219
packet-quake.c:164,186,221,258,285,293,301,349,386,410,418
packet-quake2.c:120,354,375
packet-quake3.c:178
packet-pptp.c:2123
packet-ospf.c:559
packet-giop.c:2888
packet-aim.c:732
packet-smpp.c:622,667
packet-tsp.c:176:
It looks like gryphon/packet-gryphon.c:1547 should be added to the list.
I'll work on the changes required for 2) this evening. Along with the
fixes you suggest we should probably rename "maxlength" to "buffersize" or
something similar.
Of course, we could avoid the whole mess and use safer strings in the
first place:
http://developer.gnome.org/doc/API/glib/glib-strings.html
