# [PATCH 0/2] coccinelle: detect and fix strbuf-by-value parameters 4 messages from 2026-03-15 to 2026-03-16. Participants: Deveshi Dwivedi, Junio C Hamano. Thread: https://gitlist.dev/t/65252 ## Deveshi Dwivedi, 2026-03-15 09:44 Subject: [PATCH 0/2] coccinelle: detect and fix strbuf-by-value parameters Message-ID: <20260315094445.19849-1-deveshigurgaon@gmail.com> URL: https://gitlist.dev/e/20260315094445.19849-1-deveshigurgaon%40gmail.com ``` While reviewing the write_worktree_linking_files() fix [1], Jeff King suggested adding a coccinelle rule to detect functions that take struct strbuf by value. I previously posted an RFC discussing such a rule and its implementation [2]. Patch 1/2 adds a coccinelle rule to detect functions that take struct strbuf by value and rewrites the parameter to a pointer to highlight the issue. Patch 2/2 fixes the one remaining instance found by the rule in stash.c by changing the parameter to struct strbuf * and updating the caller accordingly. The worktree.c instance that motivated the rule is already fixed by [1], so only the stash.c case remains. [1] https://lore.kernel.org/git/20260309192600.GC309867@coredump.intra.peff.net/ [2] https://lore.kernel.org/git/CAG7UgESKLMnO_4+PSJUt-TXJxFQyxEEfpCmJfMmTw2+rhT-HWw@mail.gmail.com/ Deveshi Dwivedi (2): coccinelle: detect struct strbuf passed by value stash: do not pass strbuf by value builtin/stash.c | 6 +++--- contrib/coccinelle/strbuf.cocci | 11 +++++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) -- 2.52.0.230.gd8af7cadaa ``` ## Deveshi Dwivedi, 2026-03-15 09:44 Subject: [PATCH 1/2] coccinelle: detect struct strbuf passed by value Message-ID: <20260315094445.19849-2-deveshigurgaon@gmail.com> URL: https://gitlist.dev/e/20260315094445.19849-2-deveshigurgaon%40gmail.com In-Reply-To: <20260315094445.19849-1-deveshigurgaon@gmail.com> ``` Passing a struct strbuf by value to a function copies the struct but shares the underlying character array between caller and callee. If the callee causes a reallocation, the caller's copy becomes a dangling pointer, leading to a double-free when strbuf_release() is called. There is no coccinelle rule to catch this pattern. Jeff King suggested adding one during review of the write_worktree_linking_files() fix [1], and noted that a reporting rule using coccinelle's Python scripting extensions could emit a descriptive warning, but we do not currently require Python support in coccinelle. Add a transformation rule that rewrites a by-value strbuf parameter to a pointer. The detection is identical to what a Python-based reporting rule would catch; only the presentation differs. The resulting diff will not produce compilable code on its own (callers and the function body still need updating), but the spatch output alerts the developer that the signature needs attention. This is consistent with the other rules in strbuf.cocci, which also rewrite to the preferred form. [1] https://lore.kernel.org/git/20260309192600.GC309867@coredump.intra.peff.net/ Signed-off-by: Deveshi Dwivedi --- contrib/coccinelle/strbuf.cocci | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci index 5f06105df6..83bd93be5f 100644 --- a/contrib/coccinelle/strbuf.cocci +++ b/contrib/coccinelle/strbuf.cocci @@ -60,3 +60,14 @@ expression E1, E2; @@ - strbuf_addstr(E1, real_path(E2)); + strbuf_add_real_path(E1, E2); + +@@ +identifier fn, param; +@@ + fn(..., +- struct strbuf param ++ struct strbuf *param + ,...) + { + ... + } -- 2.52.0.230.gd8af7cadaa ``` ## Deveshi Dwivedi, 2026-03-15 09:44 Subject: [PATCH 2/2] stash: do not pass strbuf by value Message-ID: <20260315094445.19849-3-deveshigurgaon@gmail.com> URL: https://gitlist.dev/e/20260315094445.19849-3-deveshigurgaon%40gmail.com In-Reply-To: <20260315094445.19849-1-deveshigurgaon@gmail.com> ``` save_untracked_files() takes its 'files' parameter as struct strbuf by value. Passing a strbuf by value copies the struct but shares the underlying buffer between caller and callee, risking a dangling pointer and double-free if the callee reallocates. The function needs both the buffer and its length for pipe_command(), so a plain const char * is not sufficient here. Switch the parameter to struct strbuf * and update the caller to pass a pointer. Signed-off-by: Deveshi Dwivedi --- builtin/stash.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index e79d612e57..472eebd6ed 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1232,7 +1232,7 @@ static int check_changes(const struct pathspec *ps, int include_untracked, } static int save_untracked_files(struct stash_info *info, struct strbuf *msg, - struct strbuf files) + struct strbuf *files) { int ret = 0; struct strbuf untracked_msg = STRBUF_INIT; @@ -1246,7 +1246,7 @@ static int save_untracked_files(struct stash_info *info, struct strbuf *msg, stash_index_path.buf); strbuf_addf(&untracked_msg, "untracked files on %s\n", msg->buf); - if (pipe_command(&cp_upd_index, files.buf, files.len, NULL, 0, + if (pipe_command(&cp_upd_index, files->buf, files->len, NULL, 0, NULL, 0)) { ret = -1; goto done; @@ -1499,7 +1499,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b parents = NULL; if (include_untracked) { - if (save_untracked_files(info, &msg, untracked_files)) { + if (save_untracked_files(info, &msg, &untracked_files)) { if (!quiet) fprintf_ln(stderr, _("Cannot save " "the untracked files")); -- 2.52.0.230.gd8af7cadaa ``` ## Junio C Hamano, 2026-03-16 15:35 Subject: Re: [PATCH 0/2] coccinelle: detect and fix strbuf-by-value parameters Message-ID: URL: https://gitlist.dev/e/xmqq5x6vrdm1.fsf%40gitster.g In-Reply-To: <20260315094445.19849-1-deveshigurgaon@gmail.com> ``` Deveshi Dwivedi writes: > While reviewing the write_worktree_linking_files() fix [1], Jeff King > suggested adding a coccinelle rule to detect functions that take > struct strbuf by value. I previously posted an RFC discussing such a rule > and its implementation [2]. > > Patch 1/2 adds a coccinelle rule to detect functions that take > struct strbuf by value and rewrites the parameter to a pointer > to highlight the issue. > > Patch 2/2 fixes the one remaining instance found by the rule in > stash.c by changing the parameter to struct strbuf * and > updating the caller accordingly. > > The worktree.c instance that motivated the rule is already fixed > by [1], so only the stash.c case remains. Nicely done. Will queue. Thanks. ```