Skip to content

Commit

Permalink
Prevent fvalue leaks
Browse files Browse the repository at this point in the history
Add a cleanup function in proto_tree_new_item for the newly
created field_info's fvalue_t, so that if we throw an exception
before adding the new node to the tree, the fvalue_t is properly
freed. Pop it off without calling it if we do add the node.

This is the function that adds nodes from values retrieved from
the tvbuffer, so it can throw exceptions, particularly on variable
length items like FT_STRINGZ, VARINTs, and FT_UINT_[STRING|BYTES].
  • Loading branch information
johnthacker committed Sep 15, 2023
1 parent 676ce39 commit d11826a
Showing 1 changed file with 20 additions and 2 deletions.
22 changes: 20 additions & 2 deletions epan/proto.c
Original file line number Diff line number Diff line change
Expand Up @@ -2599,6 +2599,13 @@ detect_trailing_stray_characters(guint encoding, const char *string, gint length
}
}

static void
free_fvalue_cb(void *data)
{
fvalue_t *fv = (fvalue_t*)data;
fvalue_free(fv);
}

/* Add an item to a proto_tree, using the text label registered to that item;
the item is extracted from the tvbuff handed to it. */
static proto_item *
Expand All @@ -2616,6 +2623,13 @@ proto_tree_new_item(field_info *new_fi, proto_tree *tree,
nstime_t time_stamp;
gboolean length_error;

/* Ensure that the newly created fvalue_t is freed if we throw an
* exception before adding it to the tree. (gcc creates clobbering
* when it optimizes the equivalent TRY..EXCEPT implementation.)
* XXX: Move the new_field_info() call inside here?
*/
CLEANUP_PUSH(free_fvalue_cb, new_fi->value);

switch (new_fi->hfinfo->type) {
case FT_NONE:
/* no value to set for FT_NONE */
Expand Down Expand Up @@ -3062,8 +3076,12 @@ proto_tree_new_item(field_info *new_fi, proto_tree *tree,

/* Don't add new node to proto_tree until now so that any exceptions
* raised by a tvbuff access method doesn't leave junk in the proto_tree. */
/* XXX. wouldn't be better to add this item to tree, with some special flag (FI_EXCEPTION?)
* to know which item caused exception? */
/* XXX. wouldn't be better to add this item to tree, with some special
* flag (FI_EXCEPTION?) to know which item caused exception? For
* strings and bytes, we would have to set new_fi->value to something
* non-NULL, or otherwise ensure that proto_item_fill_display_label
* could handle NULL values. */
CLEANUP_POP
pi = proto_tree_add_node(tree, new_fi);

switch (new_fi->hfinfo->type) {
Expand Down

0 comments on commit d11826a

Please sign in to comment.