Re: packet-daap.c issues (bugs?)

[Stephen Fisher <steve () stephen-fisher com>]

On Sun, Sep 12, 2010 at 10:23:03AM +0200, Kaul wrote:

> 1. (line 180) tagsize is an int, probably should be an unsigned int 
> (it's a length of a tag). Not sure what happens if a negative value is 
> fetched, but doesn't look right.

The tagsize is used to increment the offset at the end of 
dissect_daap_one_tag() and offsets in Wireshark are (always?) signed 
integers.  Moving the offset backwards would eventually cause an error 
for trying to access before the buffer starts in the packet.  Changing 
it to an unsigned integer (guint32) sounds good to me though, especially 
since the tvb_get_ntohl() function returns a guint32.

> 2. (line 187) it verifies tvb_offset_exists(tvb, offset) - but then 
> fetches values from 4 and 8 bytes after offset. Probably should be 
> changed to tvb_offset_exists(tvb, offset + 8) ?

It should at least be changed to check offset + 4 since the tagsize + 8 
is confirmed by a tvb_ensure_bytes_exist() call.  It is kind of hard to 
follow the code because of the two offset variables being used (offset 
and tagsize).

> 3. (line 194) After fetching tag name as a uint32, it also fetches it 
> as a string, char by char, by printing (as %c) the result of 4 
> tvb_get_guint8() calls. Strange, but legit. I want, however, to change 
> it to use value_string.

Sounds good to me.


> 4. The dissector recursively calls dissect_daap_one_tag(), I'm not 
> sure what will happen if a huge specially malformed segment (it's over 
> a HTTP response) will be dissected by Wireshark. I suspect we should 
> limit the recursion level, not only when there's nothing deeper to 
> dissect.

I don't like a function recursively calling itself because of that 
uncertainity and it makes it harder to follow the procedural flow of the 
code.  It is first called at the end of dissect_daap(), which could 
probably be modified to call it as needed (returning a varaible from the 
call for example as it already does for the offset even though that 
value is unused by the initial call).

> 5. (line 169) It seems to perform the dissection only if(tree), which 
> means it might not dissect it if the tree does not exist. Probably 
> should dissect it anyway, no?

I didn't look at what specifically could be missed by not dissecting, 
but note the paragraph in doc/README.developer that starts with "Note, 
however, that you must fill in column information, create..."  Wireshark 
will create the tree variable if any dissected information is needed 
from a particular packet, but other things such as filling in column 
information still needs to be done regardless of whether or not tree is 
non-NULL.


> 6. (line 164) It performs col_append_fstr() with 8 bytes fetched from 
> the tvb - without verifying those 8 bytes are available first. This 
> fetching is also redundant (happens again in lines 190, 191).


> I'd be happy to send patches to {some|all} the above, if people agree 
> with {some|all} my observations.

Sounds good to me.

> Hopefully, it'll help me understand why my iTunes server doesn't work 
> ;-)

That's why most of us code for Wireshark - to help ourselves first! :) 
Luckily most of us can contribute our efforts to the community at large 
afterwards.

___________________________________________________________________________
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