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.
