{"thread":{"id":"51214","subject":"[PATCH] list-objects-filter: correct usage of ALLOC_GROW","startedAt":"2019-05-31T18:46:16Z","lastAt":"2019-05-31T19:35:32Z","messageCount":2,"participants":["Matthew DeVore","Jeff Hostetler"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"376498","messageId":"20190531184606.GA29730@comcast.net","threadId":"51214","inReplyTo":null,"subject":"[PATCH] list-objects-filter: correct usage of ALLOC_GROW","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-05-31T18:46:06Z","receivedAt":"2019-05-31T18:46:16Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"In the sparse filter data, array_frame array is used in a way such that\nnr is the index of the last element. Fix this so that nr is actually the\nnumber of elements in the array.\n\nThe filter_sparse_free function also has an unaddressed TODO to free the\nmemory associated with the sparse filter data. Address that TODO and fix\nthe memory leak.\n\nSigned-off-by: Matthew DeVore <matvore@google.com>\n---\n list-objects-filter.c | 21 ++++++++++++---------\n 1 file changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/list-objects-filter.c b/list-objects-filter.c\nindex ee449de3f7..19158cc712 100644\n--- a/list-objects-filter.c\n+++ b/list-objects-filter.c\n@@ -348,28 +348,30 @@ static enum list_objects_filter_result filter_sparse(\n \tswitch (filter_situation) {\n \tdefault:\n \t\tBUG(\"unknown filter_situation: %d\", filter_situation);\n \n \tcase LOFS_BEGIN_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n \t\tdtype = DT_DIR;\n \t\tval = is_excluded_from_list(pathname, strlen(pathname),\n \t\t\t\t\t    filename, &dtype, &filter_data->el,\n \t\t\t\t\t    r->index);\n-\t\tif (val < 0)\n-\t\t\tval = filter_data->array_frame[filter_data->nr].defval;\n+\t\tif (val < 0) {\n+\t\t\tval = filter_data->array_frame[filter_data->nr - 1]\n+\t\t\t\t.defval;\n+\t\t}\n \n \t\tALLOC_GROW(filter_data->array_frame, filter_data->nr + 1,\n \t\t\t   filter_data->alloc);\n-\t\tfilter_data->nr++;\n \t\tfilter_data->array_frame[filter_data->nr].defval = val;\n \t\tfilter_data->array_frame[filter_data->nr].child_prov_omit = 0;\n+\t\tfilter_data->nr++;\n \n \t\t/*\n \t\t * A directory with this tree OID may appear in multiple\n \t\t * places in the tree. (Think of a directory move or copy,\n \t\t * with no other changes, so the OID is the same, but the\n \t\t * full pathnames of objects within this directory are new\n \t\t * and may match is_excluded() patterns differently.)\n \t\t * So we cannot mark this directory as SEEN (yet), since\n \t\t * that will prevent process_tree() from revisiting this\n \t\t * tree object with other pathname prefixes.\n@@ -380,46 +382,45 @@ static enum list_objects_filter_result filter_sparse(\n \t\t * We always show all tree objects.  A future optimization\n \t\t * may want to attempt to narrow this.\n \t\t */\n \t\tif (obj->flags & FILTER_SHOWN_BUT_REVISIT)\n \t\t\treturn LOFR_ZERO;\n \t\tobj->flags |= FILTER_SHOWN_BUT_REVISIT;\n \t\treturn LOFR_DO_SHOW;\n \n \tcase LOFS_END_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n-\t\tassert(filter_data->nr > 0);\n+\t\tassert(filter_data->nr > 1);\n \n-\t\tframe = &filter_data->array_frame[filter_data->nr];\n-\t\tfilter_data->nr--;\n+\t\tframe = &filter_data->array_frame[--filter_data->nr];\n \n \t\t/*\n \t\t * Tell our parent directory if any of our children were\n \t\t * provisionally omitted.\n \t\t */\n-\t\tfilter_data->array_frame[filter_data->nr].child_prov_omit |=\n+\t\tfilter_data->array_frame[filter_data->nr - 1].child_prov_omit |=\n \t\t\tframe->child_prov_omit;\n \n \t\t/*\n \t\t * If there are NO provisionally omitted child objects (ALL child\n \t\t * objects in this folder were INCLUDED), then we can mark the\n \t\t * folder as SEEN (so we will not have to revisit it again).\n \t\t */\n \t\tif (!frame->child_prov_omit)\n \t\t\treturn LOFR_MARK_SEEN;\n \t\treturn LOFR_ZERO;\n \n \tcase LOFS_BLOB:\n \t\tassert(obj->type == OBJ_BLOB);\n \t\tassert((obj->flags & SEEN) == 0);\n \n-\t\tframe = &filter_data->array_frame[filter_data->nr];\n+\t\tframe = &filter_data->array_frame[filter_data->nr - 1];\n \n \t\tdtype = DT_REG;\n \t\tval = is_excluded_from_list(pathname, strlen(pathname),\n \t\t\t\t\t    filename, &dtype, &filter_data->el,\n \t\t\t\t\t    r->index);\n \t\tif (val < 0)\n \t\t\tval = frame->defval;\n \t\tif (val > 0) {\n \t\t\tif (filter_data->omits)\n \t\t\t\toidset_remove(filter_data->omits, &obj->oid);\n@@ -446,39 +447,40 @@ static enum list_objects_filter_result filter_sparse(\n \t\t */\n \t\tframe->child_prov_omit = 1;\n \t\treturn LOFR_ZERO;\n \t}\n }\n \n \n static void filter_sparse_free(void *filter_data)\n {\n \tstruct filter_sparse_data *d = filter_data;\n-\t/* TODO free contents of 'd' */\n+\tfree(d->array_frame);\n \tfree(d);\n }\n \n static void *filter_sparse_oid__init(\n \tstruct oidset *omitted,\n \tstruct list_objects_filter_options *filter_options,\n \tfilter_object_fn *filter_fn,\n \tfilter_free_fn *filter_free_fn)\n {\n \tstruct filter_sparse_data *d = xcalloc(1, sizeof(*d));\n \td->omits = omitted;\n \tif (add_excludes_from_blob_to_list(filter_options->sparse_oid_value,\n \t\t\t\t\t   NULL, 0, &d->el) < 0)\n \t\tdie(\"could not load filter specification\");\n \n \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n \td->array_frame[d->nr].defval = 0; /* default to include */\n \td->array_frame[d->nr].child_prov_omit = 0;\n+\td->nr++;\n \n \t*filter_fn = filter_sparse;\n \t*filter_free_fn = filter_sparse_free;\n \treturn d;\n }\n \n static void *filter_sparse_path__init(\n \tstruct oidset *omitted,\n \tstruct list_objects_filter_options *filter_options,\n \tfilter_object_fn *filter_fn,\n@@ -486,20 +488,21 @@ static void *filter_sparse_path__init(\n {\n \tstruct filter_sparse_data *d = xcalloc(1, sizeof(*d));\n \td->omits = omitted;\n \tif (add_excludes_from_file_to_list(filter_options->sparse_path_value,\n \t\t\t\t\t   NULL, 0, &d->el, NULL) < 0)\n \t\tdie(\"could not load filter specification\");\n \n \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n \td->array_frame[d->nr].defval = 0; /* default to include */\n \td->array_frame[d->nr].child_prov_omit = 0;\n+\td->nr++;\n \n \t*filter_fn = filter_sparse;\n \t*filter_free_fn = filter_sparse_free;\n \treturn d;\n }\n \n typedef void *(*filter_init_fn)(\n \tstruct oidset *omitted,\n \tstruct list_objects_filter_options *filter_options,\n \tfilter_object_fn *filter_fn,\n-- \n2.17.1\n\n"},{"id":"376504","messageId":"de6e0e74-2388-8fef-c9c7-a7f65733c174@jeffhostetler.com","threadId":"51214","inReplyTo":"20190531184606.GA29730@comcast.net","subject":"Re: [PATCH] list-objects-filter: correct usage of ALLOC_GROW","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2019-05-31T19:35:29Z","receivedAt":"2019-05-31T19:35:32Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 5/31/2019 2:46 PM, Matthew DeVore wrote:\n> In the sparse filter data, array_frame array is used in a way such that\n> nr is the index of the last element. Fix this so that nr is actually the\n> number of elements in the array.\n\nI don't remember why I did it that way.\nThanks for fixing!\n\nJeff\n"}]}