Wireshark-dev: Re: [Wireshark-dev] [Wireshark-bugs] [Bug 7814] Buildbot crash output: fuzz-2012
On Wed, Oct 10, 2012 at 10:51 AM, Martin Mathieson
<martin.r.mathieson@xxxxxxxxxxxxxx> wrote:
> I have discovered one problem since the change, but it may have been a bug
> all along.
>
> In tcp_graph.c, it was referencing the tap (struct tcpheader) after the tap
> had run. The struct is allocated in packet-tcp.c using ep_alloc(), but now
> it wasn't valid to access that memory (immediately after tap_tcpip_packet()
> had returned). gdb reported that it wasn't valid to read that memory
> address anymore - is this a result of the change to emem.c?
Yes, although I'm not actually sure which behavior (old or new) is correct.
At this point I want to just revert the whole recent set of emem
changes *and* the ref-counting. The old, simple version has exactly
one known bug that's really hard to trigger and I think is probably a
less serious issue than the memory leak that ref-counting gave us. I
don't have access to a commit-capable machine again until tomorrow
evening, so if somebody else wants to take care of it that would be
appreciated.
Jakub, I still don't understand your proposed fix, but that's almost
certainly my fault, not yours. If it fixes the original crasher and
doesn't cause any other problems, go ahead and commit it.
> The fix (which I think I'm happy with) was to take a deep copy of the struct
> inside the tap function, i.e.
>
> Index: ui/gtk/tcp_graph.c
> ===================================================================
> --- ui/gtk/tcp_graph.c (revision 45446)
> +++ ui/gtk/tcp_graph.c (working copy)
> @@ -1885,7 +1885,10 @@
>
> /* Add address if unique and have space for it */
> if (is_unique && (th->num_hdrs < MAX_SUPPORTED_TCP_HEADERS)) {
> - th->tcphdrs[th->num_hdrs++] = header;
> + /* Need to take a deep copy of the tap struct, it may not be
> valid
> + to read after this function returns? */
> + th->tcphdrs[th->num_hdrs] = g_malloc(sizeof(struct
> tcpheader));
> + *(th->tcphdrs[th->num_hdrs++]) = *header;
> }
This is probably a good idea even with the old allocator.
