From 281dd22da96daa105580bf25f064ddfdc99a719d Mon Sep 17 00:00:00 2001 From: Peter Wu Date: Wed, 26 Sep 2018 12:26:21 +0200 Subject: [PATCH] tvb: gracefully handle reading 0 bytes from an empty buffer proto_tree_add_item with a zero length argument could end up calling tvb_get_ptr to retrieve the (empty) backing buffer. This empty tvb was possibly the result of bad reassembly, but let's gracefully handle it to avoid a dissector exception. Call trace for the original exception (only present on the first pass): proto_report_dissector_bug (format=0x7ffffffecea0 "") at epan/proto.c:1368 ensure_contiguous_no_exception (tvb=0x6060001a5460, offset=0, length=0, pexception=0x7ffffffed060) at epan/tvbuff.c:775 ensure_contiguous (tvb=0x6060001a5460, offset=0, length=0) at epan/tvbuff.c:785 tvb_get_ptr (tvb=0x6060001a5460, offset=0, length=0) at epan/tvbuff.c:906 subset_get_ptr (tvb=0x607000194b90, abs_offset=0, abs_length=0) at epan/tvbuff_subset.c:58 ensure_contiguous_no_exception (tvb=0x607000194b90, offset=0, length=0, pexception=0x7ffffffed3c0) at epan/tvbuff.c:773 ensure_contiguous (tvb=0x607000194b90, offset=0, length=0) at epan/tvbuff.c:785 tvb_get_ptr (tvb=0x607000194b90, offset=0, length=0) at epan/tvbuff.c:906 proto_tree_set_bytes_tvb (fi=0x608000535ca0, tvb=0x607000194b90, offset=0, length=0) at epan/proto.c:3862 proto_tree_new_item (new_fi=0x608000535ca0, tree=0x604000543150, tvb=0x607000194b90, start=0, length=0, encoding=0) at epan/proto.c:2318 proto_tree_add_item_new (tree=0x604000543150, hfinfo=0x7ffff30e91f8, tvb=0x607000194b90, start=0, length=0, encoding=0) at epan/proto.c:3381 proto_tree_add_item (tree=0x604000543150, hfindex=65120, tvb=0x607000194b90, start=0, length=0, encoding=0) at epan/proto.c:3391 dissect_body_data (tree=0x604000543150, pinfo=0x614000000a58, tvb=0x607000194b90, start=0, length=0, encoding=0) at epan/dissectors/packet-http2.c:1974 Change-Id: Icfae83d61ddcc9e26f16eab7f6e0e84e2f0d73ac Reviewed-on: https://code.wireshark.org/review/29851 Petri-Dish: Peter Wu Tested-by: Petri Dish Buildbot Reviewed-by: Anders Broman --- epan/tvbtest.c | 25 ++++++++++++++++++------- epan/tvbuff.c | 10 +++++++++- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/epan/tvbtest.c b/epan/tvbtest.c index 57deeba73d..fb5984d1c8 100644 --- a/epan/tvbtest.c +++ b/epan/tvbtest.c @@ -95,7 +95,7 @@ test(tvbuff_t *tvb, const gchar* name, ex_thrown = TRUE; } CATCH_ALL { - printf("02: Caught wrong exception: %lu\n", exc->except_id.except_code); + printf("03: Caught wrong exception: %lu\n", exc->except_id.except_code); } ENDTRY; @@ -121,7 +121,7 @@ test(tvbuff_t *tvb, const gchar* name, printf("04: Caught wrong exception: ReportedBoundsError\n"); } CATCH_ALL { - printf("02: Caught wrong exception: %lu\n", exc->except_id.except_code); + printf("04: Caught wrong exception: %lu\n", exc->except_id.except_code); } ENDTRY; @@ -135,7 +135,7 @@ test(tvbuff_t *tvb, const gchar* name, /* Test boundary case. A BoundsError exception should not be thrown. */ ex_thrown = FALSE; TRY { - tvb_get_ptr(tvb, 0, 1); + tvb_get_ptr(tvb, 0, length ? 1 : 0); } CATCH(BoundsError) { ex_thrown = TRUE; @@ -147,7 +147,7 @@ test(tvbuff_t *tvb, const gchar* name, printf("05: Caught wrong exception: ReportedBoundsError\n"); } CATCH_ALL { - printf("02: Caught wrong exception: %lu\n", exc->except_id.except_code); + printf("05: Caught wrong exception: %lu\n", exc->except_id.except_code); } ENDTRY; @@ -161,7 +161,7 @@ test(tvbuff_t *tvb, const gchar* name, /* Test boundary case. A BoundsError exception should not be thrown. */ ex_thrown = FALSE; TRY { - tvb_get_ptr(tvb, -1, 1); + tvb_get_ptr(tvb, -1, length ? 1 : 0); } CATCH(BoundsError) { ex_thrown = TRUE; @@ -173,7 +173,7 @@ test(tvbuff_t *tvb, const gchar* name, printf("06: Caught wrong exception: ReportedBoundsError\n"); } CATCH_ALL { - printf("02: Caught wrong exception: %lu\n", exc->except_id.except_code); + printf("06: Caught wrong exception: %lu\n", exc->except_id.except_code); } ENDTRY; @@ -258,7 +258,8 @@ test(tvbuff_t *tvb, const gchar* name, /* One big memdup */ ptr = (guint8*)tvb_memdup(NULL, tvb, 0, -1); - if (memcmp(ptr, expected_data, length) != 0) { + if ((length != 0 && memcmp(ptr, expected_data, length) != 0) || + (length == 0 && ptr != NULL)) { printf("12: Failed TVB=%s Offset=0 Length=-1 " "Bad memdup\n", name); failed = TRUE; @@ -279,9 +280,11 @@ run_tests(void) int i, j; tvbuff_t *tvb_parent; + tvbuff_t *tvb_empty; tvbuff_t *tvb_small[3]; tvbuff_t *tvb_large[3]; tvbuff_t *tvb_subset[6]; + tvbuff_t *tvb_empty_subset; guint8 *small[3]; guint small_length[3]; guint small_reported_length[3]; @@ -326,6 +329,10 @@ run_tests(void) tvb_set_free_cb(tvb_large[i], g_free); } + /* Test empty tvb */ + tvb_empty = tvb_new_child_real_data(tvb_parent, NULL, 0, 1); + test(tvb_empty, "Empty", NULL, 0, 1); + /* Test the "real" tvbuff objects. */ test(tvb_small[0], "Small 0", small[0], small_length[0], small_reported_length[0]); test(tvb_small[1], "Small 1", small[1], small_length[1], small_reported_length[1]); @@ -373,6 +380,10 @@ run_tests(void) test(tvb_subset[4], "Subset 4", subset[4], subset_length[4], subset_reported_length[4]); test(tvb_subset[5], "Subset 5", subset[5], subset_length[5], subset_reported_length[5]); + /* Subset of an empty tvb. */ + tvb_empty_subset = tvb_new_subset_length_caplen(tvb_empty, 0, 0, 1); + test(tvb_empty_subset, "Empty Subset", NULL, 0, 1); + /* One Real */ printf("Making Composite 0\n"); tvb_comp[0] = tvb_new_composite(); diff --git a/epan/tvbuff.c b/epan/tvbuff.c index e40401075a..c759b91a26 100644 --- a/epan/tvbuff.c +++ b/epan/tvbuff.c @@ -762,6 +762,14 @@ ensure_contiguous_no_exception(tvbuff_t *tvb, const gint offset, const gint leng return NULL; } + /* + * Special case: if the caller (e.g. tvb_get_ptr) requested no data, + * then it is acceptable to have an empty tvb (!tvb->real_data). + */ + if (length == 0) { + return NULL; + } + /* * We know that all the data is present in the tvbuff, so * no exceptions should be thrown. @@ -783,7 +791,7 @@ ensure_contiguous(tvbuff_t *tvb, const gint offset, const gint length) const guint8 *p; p = ensure_contiguous_no_exception(tvb, offset, length, &exception); - if (p == NULL) { + if (p == NULL && length != 0) { DISSECTOR_ASSERT(exception > 0); THROW(exception); }