{"thread":{"id":"65252","subject":"[PATCH 0/2] coccinelle: detect and fix strbuf-by-value parameters","startedAt":"2026-03-15T09:44:53Z","lastAt":"2026-03-16T15:35:52Z","messageCount":4,"participants":["Deveshi Dwivedi","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"539018","messageId":"20260315094445.19849-1-deveshigurgaon@gmail.com","threadId":"65252","inReplyTo":null,"subject":"[PATCH 0/2] coccinelle: detect and fix strbuf-by-value parameters","fromName":"Deveshi Dwivedi","fromEmail":"deveshigurgaon@gmail.com","sentAt":"2026-03-15T09:44:42Z","receivedAt":"2026-03-15T09:44:53Z","isPatch":true,"sender":{"key":"deveshigurgaon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/120312681?v=4"},"body":"While reviewing the write_worktree_linking_files() fix [1], Jeff King\nsuggested adding a coccinelle rule to detect functions that take\nstruct strbuf by value.  I previously posted an RFC discussing such a rule\nand its implementation [2].\n\nPatch 1/2 adds a coccinelle rule to detect functions that take\nstruct strbuf by value and rewrites the parameter to a pointer\nto highlight the issue.\n\nPatch 2/2 fixes the one remaining instance found by the rule in\nstash.c by changing the parameter to struct strbuf * and\nupdating the caller accordingly.\n\nThe worktree.c instance that motivated the rule is already fixed\nby [1], so only the stash.c case remains.\n\n[1] https://lore.kernel.org/git/20260309192600.GC309867@coredump.intra.peff.net/\n[2] https://lore.kernel.org/git/CAG7UgESKLMnO_4+PSJUt-TXJxFQyxEEfpCmJfMmTw2+rhT-HWw@mail.gmail.com/\n\nDeveshi Dwivedi (2):\n  coccinelle: detect struct strbuf passed by value\n  stash: do not pass strbuf by value\n\n builtin/stash.c                 |  6 +++---\n contrib/coccinelle/strbuf.cocci | 11 +++++++++++\n 2 files changed, 14 insertions(+), 3 deletions(-)\n\n-- \n2.52.0.230.gd8af7cadaa\n\n"},{"id":"539019","messageId":"20260315094445.19849-2-deveshigurgaon@gmail.com","threadId":"65252","inReplyTo":"20260315094445.19849-1-deveshigurgaon@gmail.com","subject":"[PATCH 1/2] coccinelle: detect struct strbuf passed by value","fromName":"Deveshi Dwivedi","fromEmail":"deveshigurgaon@gmail.com","sentAt":"2026-03-15T09:44:43Z","receivedAt":"2026-03-15T09:44:56Z","isPatch":true,"sender":{"key":"deveshigurgaon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/120312681?v=4"},"body":"Passing a struct strbuf by value to a function copies the struct\nbut shares the underlying character array between caller and callee.\nIf the callee causes a reallocation, the caller's copy becomes a\ndangling pointer, leading to a double-free when strbuf_release() is\ncalled.  There is no coccinelle rule to catch this pattern.\n\nJeff King suggested adding one during review of the\nwrite_worktree_linking_files() fix [1], and noted that a reporting\nrule using coccinelle's Python scripting extensions could emit a\ndescriptive warning, but we do not currently require Python support\nin coccinelle.\n\nAdd a transformation rule that rewrites a by-value strbuf parameter\nto a pointer.  The detection is identical to what a Python-based\nreporting rule would catch; only the presentation differs.  The\nresulting diff will not produce compilable code on its own (callers\nand the function body still need updating), but the spatch output\nalerts the developer that the signature needs attention.  This is\nconsistent with the other rules in strbuf.cocci, which also rewrite\nto the preferred form.\n\n[1] https://lore.kernel.org/git/20260309192600.GC309867@coredump.intra.peff.net/\n\nSigned-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>\n---\n contrib/coccinelle/strbuf.cocci | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci\nindex 5f06105df6..83bd93be5f 100644\n--- a/contrib/coccinelle/strbuf.cocci\n+++ b/contrib/coccinelle/strbuf.cocci\n@@ -60,3 +60,14 @@ expression E1, E2;\n @@\n - strbuf_addstr(E1, real_path(E2));\n + strbuf_add_real_path(E1, E2);\n+\n+@@\n+identifier fn, param;\n+@@\n+  fn(...,\n+- struct strbuf param\n++ struct strbuf *param\n+  ,...)\n+  {\n+  ...\n+  }\n-- \n2.52.0.230.gd8af7cadaa\n\n"},{"id":"539020","messageId":"20260315094445.19849-3-deveshigurgaon@gmail.com","threadId":"65252","inReplyTo":"20260315094445.19849-1-deveshigurgaon@gmail.com","subject":"[PATCH 2/2] stash: do not pass strbuf by value","fromName":"Deveshi Dwivedi","fromEmail":"deveshigurgaon@gmail.com","sentAt":"2026-03-15T09:44:44Z","receivedAt":"2026-03-15T09:44:59Z","isPatch":true,"sender":{"key":"deveshigurgaon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/120312681?v=4"},"body":"save_untracked_files() takes its 'files' parameter as struct strbuf\nby value.  Passing a strbuf by value copies the struct but shares\nthe underlying buffer between caller and callee, risking a dangling\npointer and double-free if the callee reallocates.\n\nThe function needs both the buffer and its length for\npipe_command(), so a plain const char * is not sufficient here.\nSwitch the parameter to struct strbuf * and update the caller to\npass a pointer.\n\nSigned-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>\n---\n builtin/stash.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex e79d612e57..472eebd6ed 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1232,7 +1232,7 @@ static int check_changes(const struct pathspec *ps, int include_untracked,\n }\n \n static int save_untracked_files(struct stash_info *info, struct strbuf *msg,\n-\t\t\t\tstruct strbuf files)\n+\t\t\t\tstruct strbuf *files)\n {\n \tint ret = 0;\n \tstruct strbuf untracked_msg = STRBUF_INIT;\n@@ -1246,7 +1246,7 @@ static int save_untracked_files(struct stash_info *info, struct strbuf *msg,\n \t\t\t stash_index_path.buf);\n \n \tstrbuf_addf(&untracked_msg, \"untracked files on %s\\n\", msg->buf);\n-\tif (pipe_command(&cp_upd_index, files.buf, files.len, NULL, 0,\n+\tif (pipe_command(&cp_upd_index, files->buf, files->len, NULL, 0,\n \t\t\t NULL, 0)) {\n \t\tret = -1;\n \t\tgoto done;\n@@ -1499,7 +1499,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b\n \tparents = NULL;\n \n \tif (include_untracked) {\n-\t\tif (save_untracked_files(info, &msg, untracked_files)) {\n+\t\tif (save_untracked_files(info, &msg, &untracked_files)) {\n \t\t\tif (!quiet)\n \t\t\t\tfprintf_ln(stderr, _(\"Cannot save \"\n \t\t\t\t\t\t     \"the untracked files\"));\n-- \n2.52.0.230.gd8af7cadaa\n\n"},{"id":"539115","messageId":"xmqq5x6vrdm1.fsf@gitster.g","threadId":"65252","inReplyTo":"20260315094445.19849-1-deveshigurgaon@gmail.com","subject":"Re: [PATCH 0/2] coccinelle: detect and fix strbuf-by-value parameters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-16T15:35:50Z","receivedAt":"2026-03-16T15:35:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Deveshi Dwivedi <deveshigurgaon@gmail.com> writes:\n\n> While reviewing the write_worktree_linking_files() fix [1], Jeff King\n> suggested adding a coccinelle rule to detect functions that take\n> struct strbuf by value.  I previously posted an RFC discussing such a rule\n> and its implementation [2].\n>\n> Patch 1/2 adds a coccinelle rule to detect functions that take\n> struct strbuf by value and rewrites the parameter to a pointer\n> to highlight the issue.\n>\n> Patch 2/2 fixes the one remaining instance found by the rule in\n> stash.c by changing the parameter to struct strbuf * and\n> updating the caller accordingly.\n>\n> The worktree.c instance that motivated the rule is already fixed\n> by [1], so only the stash.c case remains.\n\nNicely done.  Will queue.  Thanks.\n"}]}