Re: tvb_get_nstringz0

[Dario Lombardo <lomato () gmail com>]

Hi, John, thanks for the follow-up. I have used gdb but I didn't hit any
failed assertion. I will file a bug with the test sample and reference you
in it so you can have a clearer view of what's going on.

On Sun, Mar 28, 2021 at 1:33 AM John Thacker <johnthacker () gmail com> wrote:

> On Sat, Mar 27, 2021 at 2:57 PM Dario Lombardo <lomato () gmail com> wrote:
>
> > Hi John,
> > thanks, your explanation helped a lot. However I still don't get why the
> > code crashes. Please let me use the actual buffer sizes since the ones I
> > told before were examples. The packet is 49, the local buffer is 15.
> >
> > When you call tvb_get_nstringz0() you pass in bufsize = 15.
> > > tvb_get_nstringz0() calls _tvb_get_nstringz()
> > > check_offset_len() runs to the end of the packet, setting len to 49.
> > > Since len >= bufsize, it sets limit = bufsize.
> > > stringlen = tvb_strnlen(tvb, abs_offset, limit - 1) looks at the first 9
> > > bytes, doesn't find a NUL, returns -1
> >
> > That's a point I don't get. This piece of code (stringlen =
> > tvb_strnlen(tvb, 0, 14)) actually returns 49. Despite the fact that NULL is
> > present or not, shouldn't this function fulfill the (limit - 1)? Shouldn't
> > that return 14 at most?
> >
> >
> > > stringlen is -1, tvb_memcpy copies over limit (10) bytes into buffer
> > > from tvb, bytes_copies is set to 10, _tvb_get_nstringz() returns -1.
> >
> > That's where things start to get hairy: stringlen is 49, then the actual
> > copy starts against buffer, that is only 15 bytes long. Crash.
>
> Huh, that's odd. I just added some lines to packet-http.c near the top of
> dissect_http_tcp() like so:
>
>         guint bufsize = 19;
>         guint8 buffer[19];
>         tvb_get_nstringz0(tvb, 0, bufsize, buffer);
>
> And stepped through with gdb after setting a breakpoint and opening a file
> with a ~480 byte HTTP packet with no nulls in the HTTP layer:
>
> 3578 stringlen = tvb_strnlen(tvb, abs_offset, limit - 1);
> (gdb) print limit -1
> $12 = 18
> (gdb) n
> 3580 if (stringlen == -1) {
> (gdb) print stringlen
> $14 = -1
> (gdb) print tvb->length
> $15 = 479
>
> And all proceeds as expected.
>
> tvb_strnlen(tvb, offset, maxlength) is supposed to return -1 if
> 'maxlength' (here, 14) is reached before a NULL.
>
> Let's see, tvb_strnlen calls tvb_find_guint8(tvb, abs_offset,
> maxlength=14, 0), and returns -1 if and only if tvb_find_guint8(...)
> returns -1. That looks right, so let's look at tvb_find_guint8():
>
>         DISSECTOR_ASSERT(tvb && tvb->initialized);
>
>         exception = compute_offset_and_remaining(tvb, offset, &abs_offset,
> &limit);
>         if (exception)
>                 THROW(exception);
>
>         /* Only search to end of tvbuff, w/o throwing exception. */
>         if (maxlength >= 0 && limit > (guint) maxlength) {
>                 /* Maximum length doesn't go past end of tvbuff; search
>                    to that value. */
>                 limit = (guint) maxlength;
>         }
>
> Have you tried stepping through it with a debugger? The code looks right
> to me and runs correctly over here. Are you perhaps somehow hitting an
> exception or a failed assertion?
>
> John
>
> ___________________________________________________________________________
> 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



-- 

Naima is online.

___________________________________________________________________________
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