git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 1/3] line-log: protect inner strbuf from free

From
Patrick Steinhardt <ps@pks.im>
Date
Oct 4, 2024, 04:32 UTC
Message-ID
<Zv9v8ZKIgQpxpuVv@pks.im>
In-Reply-To
<05c21616c350b5141c17fde1aa5d3aea881c6031.1727956724.git.gitgitgadget@gmail.com>
On Thu, Oct 03, 2024 at 11:58:42AM +0000, Derrick Stolee via GitGitGadget wrote:
Show 34 quoted lines
> From: Derrick Stolee <stolee@gmail.com>
> 
> The output_prefix() method in line-log.c may call a function pointer via
> the diff_options struct. This function pointer returns a strbuf struct
> and then its buffer is passed back. However, that implies that the
> consumer is responsible to free the string. This is especially true
> because the default behavior is to duplicate the empty string.
> 
> The existing functions used in the output_prefix pointer include:
> 
>  1. idiff_prefix_cb() in diff-lib.c. This returns the data pointer, so
>     the value exists across multiple calls.
> 
>  2. diff_output_prefix_callback() in graph.c. This uses a static strbuf
>     struct, so it reuses buffers across calls. These should not be
>     freed.
> 
>  3. output_prefix_cb() in range-diff.c. This is similar to the
>     diff-lib.c case.
> 
> In each case, we should not be freeing this buffer. We can convert the
> output_prefix() function to return a const char pointer and stop freeing
> the result.
> 
> This choice is essentially the opposite of what was done in 394affd46d
> (line-log: always allocate the output prefix, 2024-06-07).
> 
> This was discovered via 'valgrind' while investigating a public report
> of a bug in 'git log --graph -L' [1].
> 
> [1] https://github.com/git-for-windows/git/issues/5185
> 
> This issue would have been caught by the new test, when Git is compiled
> with ASan to catch these double frees.
Thanks a bunch for fixing this! The change looks good to me.
Patrick
Previous: Derrick Stolee via GitGitGadgetNext: Jeff King via GitGitGadget
Message 10 of 24 in “line-log: protect inner strbuf from free”
  1. line-log: protect inner strbuf from freeDerrick Stolee via GitGitGadget, Oct 2, 2024
  2. Jeff KingOct 2, 2024
  3. Jeff KingOct 3, 2024
  4. Derrick StoleeOct 3, 2024
  5. Jeff KingOct 3, 2024
  6. Derrick StoleeOct 3, 2024
  7. Jeff KingOct 3, 2024
  8. 0/3 line-log: protect inner strbuf from freeDerrick Stolee via GitGitGadget, Oct 3, 2024
  9. 1/3 line-log: protect inner strbuf from freeDerrick Stolee via GitGitGadget, Oct 3, 2024
  10. Patrick SteinhardtOct 4, 2024
  11. 2/3 line-log: remove output_prefix()Jeff King via GitGitGadget, Oct 3, 2024
  12. 3/3 diff: modify output_prefix function pointerJeff King via GitGitGadget, Oct 3, 2024
  13. Junio C HamanoOct 3, 2024
  14. 0/5 diff output_prefix cleanupsJeff King, Oct 3, 2024
  15. 1/5 line-log: use diff_line_prefix() instead of custom helperJeff King, Oct 3, 2024
  16. 2/5 diff: drop line_prefix_length fieldJeff King, Oct 3, 2024
  17. 3/5 diff: return const char from output_prefix callbackJeff King, Oct 3, 2024
  18. 4/5 diff: return line_prefix directly when possibleJeff King, Oct 3, 2024
  19. 5/5 diff: store graph prefix buf in git_graph structJeff King, Oct 3, 2024
  20. Junio C HamanoOct 3, 2024
  21. Patrick SteinhardtOct 4, 2024
  22. Derrick StoleeOct 4, 2024
  23. Junio C HamanoOct 4, 2024
  24. Derrick StoleeOct 4, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.