Wireshark-bugs: [Wireshark-bugs] [Bug 9323] Buildbot crash output: fuzz-2013-10-25-12569.pcap
Comment # 9
on bug 9323
from Evan Huus
(In reply to comment #7)
> Ermm, isn't the problem that HTTP is adding an FT_STRING ("not necessarily
> NULL terminated" according to README.dissector) but proto_tree_add_string()
> + proto_tree_set_string() are assuming that it IS NULL terminated (the
> length isn't even passed in to the latter function)?
Ya, this is a much better explanation. The reason it isn't terminated is
because it's from tvb_get_ptr and the packet has no NULLs past that point, but
that's really secondary.
> If that's the case,
> have we really been living with non-NULL-terminated FT_STRINGs having this
> problem for, like, ever?
Yup. Fortunately since wiretap overallocates and that memory usually has a null
in it somewhere, we've never seen any bugs from it. It also helps that the
majority of strings passed to proto_tree_set_string don't come from
tvb_get_ptr, and so *are* guaranteed to be null-terminated (since
tvb_get_string et al do that).
> (In reply to comment #6)
> > This one's weird. The root problem, I think, is that the last line of HTTP
> > header is being added to the tree as a pointer retrieved with tvb_get_ptr,
> > however the line contained no null terminators. The tree is strduping it for
> > display, which of course runs past the end of the packet into uninitialized
> > wiretap buffer.
>
> Which reminds me: years ago after I had added the memory scrubbing, I wanted
> to add scrubbing to the wiretap buffers too.
If wiretap continues to over-allocate buffers (which it may need to for
performance reasons) then it will probably have to handle its own scrubbing. If
it is converted to use wmem and only allocate what it needs then it gets
scrubbing more-or-less for free. Using a wmem block allocator may provide the
same performance as over-allocating, I'm really not sure what the logic was
there.
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > I tried to modify the function to use g_strlcpy in order to respect both the
> > > length *and* any possible null-terminator but that just moved the source of
> > > the error to the g_strlcpy call. That makes me suspect the length being
> > > passed in is incorrect, but I cannot track down how.
> >
> > I didn't try that yet but the length looks OK to me (length is 48 though the
> > 'line' is "Accept-Charset" through the end of the packet and into wiretap's
> > unused space).
>
> Actually the problem there is in the g_strlcpy() documentation. To quote:
>
> "src must be nul-terminated;"!
Dangit, I assumed that because "At most dest_size - 1 characters will be
copied" it didn't need to be null-terminated if it was longer than that. Welp.
> Gotta use strncpy. This patch fixes it though of course it needs to be done
> properly... Maybe after a game of soccer (football) with the kids...
>
> ~~~
> @@ -2590,10 +2591,13 @@
>
> /* Set the FT_STRING value */
> static void
> -proto_tree_set_string(field_info *fi, const char* value)
> +proto_tree_set_string(field_info *fi, const char* value, gint length)
> {
> if (value) {
> - fvalue_set(&fi->value, (gpointer) value, FALSE);
> + /*fvalue_set(&fi->value, (gpointer) value, FALSE);*/
> + fi->value.value.string = (gchar *)g_malloc(length+1);
> + strncpy(fi->value.value.string, value, length);
> + fi->value.value.string[length] = 0;
> } else {
> fvalue_set(&fi->value, (gpointer) "[ Null ]", FALSE);
> }
> ~~~
I think the best way to properly include this logic is to enhance
string_fvalue_set (ftype-string.c) to correctly handle already_copied==TRUE
(instead of asserting), then use g_strndup (simpler than g_malloc+strncpy) in
proto_tree_set_string and pass that into fvalue_set() with already_copied=TRUE.
You are receiving this mail because:
- You are watching all bug changes.
