Patrick Steinhardt <ps@xxxxxx> writes: > When calling `git describe --contains=`, we end up invoking > `cmd_name_rev()` with some munged argv array. This array may contain > allocated strings and furthermore will likely be modified by the called > function. This results in two memory leaks: > > - First, we leak the array that we use to assemble the arguments. > > - Second, we leak the allocated strings that we may have put into the > array. > > Fix those leaks by creating a separate copy of the array that we can > hand over to `cmd_name_rev()`. This allows us to free all strings > contained in the `strvec`, as the original vector will not be modified > anymore. OK, the separate copy has to be a shallow copy, as its purpose is not to lose pointers to the contained strings. > Furthermore, free both the `strvec` and the copied array to fix the > first memory leak. > ... > + strvec_clear(&args); > + free(argv_copy); So, calling cmd_name_rev() may shuffle the argv_copy[] array but at least it will not free any element in it (as expected---it is typically the (argc, argv[]) the process receives from getting exec'ed) [*]. Freeing the argv_copy shell itself is sufficient to discard what we used to call cmd_name_rev(). And we discard args both its content strings and the array. OK. > + return ret; > } > > hashmap_init(&names, commit_name_neq, NULL, 0); [Footnote] * The fact that cmd_foo() is called is not a hygiene thing to do in the first place, and in the longer term #leftoverbits we may need to refactor the thing further, into a proper library-ish reusable helper function that can be used to compute name_rev() any number of times, plus cmd_name_rev() and this caller that call it.
