[PATCH 0/2] coccinelle: detect and fix strbuf-by-value parameters

[PATCH 0/2] coccinelle: detect and fix strbuf-by-value parameters

[Deveshi Dwivedi]

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

[PATCH 1/2] coccinelle: detect struct strbuf passed by value

[Deveshi Dwivedi]

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 <deveshigurgaon@gmail.com>
---
 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

[PATCH 2/2] stash: do not pass strbuf by value

[Deveshi Dwivedi]

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 <deveshigurgaon@gmail.com>
---
 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

Re: [PATCH 0/2] coccinelle: detect and fix strbuf-by-value parameters

[Junio C Hamano]

Deveshi Dwivedi <deveshigurgaon@gmail.com> 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.