Wireshark-dev: Re: [Wireshark-dev] pcapng, must opt_comment string be?0-terminated?
Thus wrote Chris Maynard (Chris.Maynard@xxxxxxxxx):
> To avoid the possibility of reading past the end of option_content in
> the case when oh.option_length > sizeof(option_content), I think it
> would be safer to do this instead:
> wblock->data.packet.opt_comment = g_strndup(option_content,
> MIN(oh.option_length, sizeof(option_content)))
> Note that this can't happen today because if the option length is greater than
> sizeof(option_content), then we never get there,
yes, I saw the check before the g_strndup()
> but I think the implementation would work better to read as much as
> will fit into the buffer as possible, in this case the 1st 99
> characters of the string (plus the NULL-terminator), rather than
> skipping it altogether. The comment next to option_content indicates,
> "XXX - size might need to be increased, if we see longer options", so
> it's apparently a somewhat arbitrary limit. I think as pcapng sees
> more and more use, it's only a matter of time before longer and longer
> strings are added (or desired to be added). Even the examples shown
> at the end of section 2.5 of the pcapng spec illustrate some strings
> approaching that limit, and those strings do not seem overly long to
> me:
> "This packet is the beginning of all of our problems" / "Packets 17-23 showing a
> bogus TCP retransmission, as reported in bugzilla entry 1486!" / "Captured at
> the southern plant" / "I've checked again, now it's working ok" / ...
You're right, 100 characters seems arbitrary and probably too small for
many cases.
I was wondering why we need a static buffer at all. It looks like the
intention is to keep using the same buffer for each option that we
parse. When reading an option, how about checking the length first and
then allocating the buffer dynamically? We could then remove the
g_strdup() as well and use the allocated buffer to pass the option on to
wiretap etc.
My understandig is that g_strdup() allocates a copy that the caller must
free. I don't think that at the moment, anybody is freeing the copy for
the comment (or for any other option).
Best regards,
Martin
