Wireshark mailing list archives
Re: pcapng, must opt_comment string be?0-terminated?
From: Martin Kaiser <lists () kaiser cx>
Date: Thu, 12 Jan 2012 22:30:18 +0100
Thus wrote Chris Maynard (Chris.Maynard () gtech com):
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 ___________________________________________________________________________ Sent via: Wireshark-dev mailing list <wireshark-dev () wireshark org> Archives: http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-request () wireshark org?subject=unsubscribe
Current thread:
- Re: pcapng, must opt_comment string be?0-terminated? Martin Kaiser (Jan 12)
- Re: [Wireshark-dev] pcapng, must opt_commentstring be?0-terminated? Chris Maynard (Jan 23)
- Re: pcapng, must opt_commentstring?be?0-terminated? Martin Kaiser (Jan 29)
- Re: [Wireshark-dev] pcapng, must opt_commentstring?be?0-terminated? Chris Maynard (Jan 29)
- Re: pcapng, must opt_commentstring?be?0-terminated? Martin Kaiser (Jan 29)
- Re: [Wireshark-dev] pcapng, must opt_commentstring be?0-terminated? Chris Maynard (Jan 23)