Wireshark-bugs: [Wireshark-bugs] [Bug 3290] TRY_TO_FAKE_THIS_ITEM disables bounds errors

Date: Thu, 5 Mar 2009 22:58:02 -0800 (PST)
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=3290





--- Comment #6 from didier gautheron <dgautheron@xxxxxxxx>  2009-03-05 22:57:58 PDT ---
(In reply to comment #4)
> So here's my analysis of the problem.
> 
> The problem is not the if(tree)s (since tree will be set whenever we have a
> filter) but TRY_TO_FAKE_THIS_ITEM which avoids building items (including
> checking for bounds errors) unless they are referenced by a filter.
> 
> Unless I'm wrong, that means we haven't been getting bounds errors from
> proto_tree_add_*() since TRY_TO_FAKE_THIS_ITEM went in.  (I'm a little
> uncomfortable asserting that since that means all the exceptions we /do/ get
> are from tvb_*() accesses, but...)
> 
> I think Didier's patch makes sense but it would have to be propagated to all
> the other calls too.
In my understanding following functions can throw an exception: 
 always
   ptvcursor_add()
   proto_tree_add_item()
   proto_tree_add_protocol_format()
   proto_tree_add_none_format()

 if length < 0 
     FT_BYTES, FT_STRING
   proto_tree_add_bytes()
   proto_tree_add_string()
   but here it's likely a dissector bug.

and all other functions if length <= -1, this case needs more analysis, ie for
FT_UINT it can throw DISSECTOR_ASSERT_NOT_REACHED() or a ReportedBoundsError.

> 
> Question: should we be doing bounds checking even when we're not building the
> tree?  Didier's patch does this but I'm split on whether it should.  In the

The patch is wrong because it doesn't do the right check for FT_UINT_BYTES and
FT_UINT_STRING, cf ptvcursor_add()

> "old days" when dissectors had to check the tree we wouldn't have been, but I
> do dislike the idea of things changing based on whether a filter is set.
> 


-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.