TRY_TO_FAKE_THIS_ITEM disables bounds errors

This issue was migrated from bug 3290 in our old bug tracker.

Original bug information:

Reporter: didier gautheron
Status: CONFIRMED
Product: Wireshark
Component: Dissection engine (libwireshark)
OS: All
Platform: All
Version: Git

Attachments:

always_ensures_bytes_exist_in_proto_add_item.diff: always test for bound errors in proto_add_item()
malformed_packet_not_selected.pcap: Malformed AFP packet not selected by 'malformed' filter
move_tree_NULL_test_in_TRY_TO_FAKE_THIS_ITEM_macro.diff: Move tree NULL test in TRY_TO_FAKE_THIS_ITEM
test_bound_errors_even_if_tree_is_NULL_or_fake_item.diff: Test bound errors even if tree is NULL when needed
test_bound_error_in_proto_tree_add_bits_item.diff: test bound errors in proto_tree_add_bits_xxx when tree is null
test_bound_errors_even_if_tree_is_NULL_or_fake_item_v2_.diff: Test bound errors
nfs-create-session-long-loop-no-exception.pcap: nfs: no exception without tree

See also: Issue #8364 (closed)
Issue #11358 (closed)

To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information

Child items 0

No child items are currently assigned. Use child items to break down this issue into smaller parts.

Hi, It's an issue if the error is in the last proto_add_item() call, the packet throw an exception if selected in the first pane but 'malformed' filter fails. I suspect it's bug #3285 cause. Attached an other example with AFP and a patch. I'm not sure about the patch, it works for me though.

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. 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 "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.

(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. >

From bug 4060 which was marked as a dup of bug 3285 (which is a dup of this bug): > [Note that bug 3290 indicates that proto_tree_add*() don't really generate > > exceptions any more.] I'm not sure I understand "proto_tree_add*() don't really generate exceptions any more" since the "Malformed packet (Exception occurred)" does show in the packet details. ... Maybe my understanding of this bug isn't quite right.

Created attachment 8729 nfs: no exception without tree Opening attached file takes much more time if there's no tree (without -V). In dissect_rpc_chanattrs4() there's nice loop: for (i = 0; i < rdma_ird_len; i++) offset = dissect_rpc_uint32(tvb, tree, hf_nfs_rdmachanattrs4, offset); Which won't be terminated with offset out of bounds when tree == NULL. (Previously found in bug #7436) If there's tree but only for display filter (-R tcp), it'll still not throw exception, but it'll terminate on assert: [Dissector bug, protocol NFS: More than 1000000 items in the tree -- possible infinite loop]

TRY_TO_FAKE_THIS_ITEM disables bounds errors

Activity