FT_STRING, FT_STRINGZPAD, and null padding

[John Thacker <johnthacker () gmail com>]

I don't understand this change, which makes me wonder if I understand
FT_STRINGZPAD vs FT_STRING in general:
https://gitlab.com/wireshark/wireshark/-/commit/a42286524a0e23f5944e988e672a07a5fb395c69


Everything in proto.c and tvbuff.c treats FT_STRING and FT_STRINGZPAD
exactly the same currently, except for checking stray characters. If the
length is given as -1, the length to the end of the tvbuff is used.
Otherwise, the specified length is checked and copied from the tvbuff, and
a NUL is added at the end regardless. The last byte ensures that they both
work even if the buffer is not null terminated.

However, if the string is null terminated somewhere in the middle, all the
bytes after that (presumably padding) are still copied into the return
value, along with the extra '\0'. This doesn't really cause any problems
other than wasting a little bit of memory and time, since the first null
ends the string.

Before that commit, there was absolutely no difference that I could tell.
Now the difference is that if the string is terminated by a NUL somewhere
in the middle before the specified field length, whether or not the bytes
in the padding after that must be null padding. It is FT_STRING that
requires that any padding be zero-padding (and adds an expert warning if
not), whereas FT_STRINGZPAD does not.

While I agree with the commit message that "there are protocols where a
string that's not the full length of the part of the packet for the string
has a null terminator but isn't guaranteed to be fully padded with nulls,"
and that there can be "a separate type for fields where we really *should*
check that the padding is all nulls," I don't see why FT_STRINGZPAD isn't
that latter type. To me, the name FT_STRINGZPAD implies that it is the one
where we really should check that the padding is zero padding.

I would reverse the handling in that commit, and have FT_STRINGZPAD be the
type that does check, and make FT_STRING be the type that does not check
for trailing characters.

The documentation in README.dissector does not really explain the
differences between the use cases, as it seems like FT_STRING can be used
for everything where FT_STRINGZPAD can:

    FT_STRING   A string of characters, not necessarily
NULL-terminated, but possibly NULL-padded.                This, and
the other string-of-characters                types, are to be used
for text strings,                not raw binary data.
...    FT_STRINGZPAD   A NULL-padded string of characters.
   The length is given in the proto_tree_add_item()
call, but may be larger than the length of                the string,
with extra bytes being NULL padding.                This is typically
used for fixed-length fields                that contain a string
value that might be shorter                than the fixed length.

John Thacker

___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe