Re: [PATCH 20/29] sequencer: fix leaking string buffer in `commit_staged_changes()`

From: Patrick Steinhardt <ps@pks.im>
Date: Jun 4, 2024, 07:19 UTC
Message-ID: <Zl6_6Z6Bu-zgOmPs@framework>
In-Reply-To: <Zl6390B6kUu5TlBM@framework>

On Tue, Jun 04, 2024 at 08:45:11AM +0200, Patrick Steinhardt wrote:

Show 21 quoted lines

> On Mon, Jun 03, 2024 at 02:14:20PM +0100, Phillip Wood wrote:
> > Hi Patrick
> > 
> > On 03/06/2024 10:48, Patrick Steinhardt wrote:
> > > @@ -5259,12 +5277,13 @@ static int commit_staged_changes(struct repository *r,
> > >   				}
> > >   			unuse_commit_buffer:
> > >   				repo_unuse_commit_buffer(r, commit, p);
> > > -				if (res)
> > > -					return res;
> > > +				if (res) {
> > > +					ret = res;
> > > +					goto out;
> > > +				}
> > 
> > Having 'ret' and 'res' in this block is a bit confusing - we could delete
> > the declaration for 'res' and  either replace its use with 'ret', or rename
> > 'ret' to 'res' in this patch.
> 
> Yeah, let's just drop the local `res` variable here and use `ret`
> instead.

For the record, below is the diff to address this. I'll refrain from sending out this whole patch series again for now to only address this one issue, but will include it if there are more things that need to be handled.

Patrick

diff --git a/sequencer.c b/sequencer.c
index 9e90084692..cc57a30883 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5253,7 +5253,6 @@ static int commit_staged_changes(struct repository *r,
 				 * We need to update the squash message to skip
 				 * the latest commit message.
 				 */
-				int res = 0;
 				struct commit *commit;
 				const char *msg;
 				const char *path = rebase_path_squash_msg();
@@ -5266,22 +5265,22 @@ static int commit_staged_changes(struct repository *r,
 
 				p = repo_logmsg_reencode(r, commit, NULL, encoding);
 				if (!p)  {
-					res = error(_("could not parse commit %s"),
+					ret = error(_("could not parse commit %s"),
 						    oid_to_hex(&commit->object.oid));
 					goto unuse_commit_buffer;
 				}
 				find_commit_subject(p, &msg);
 				if (write_message(msg, strlen(msg), path, 0)) {
-					res = error(_("could not write file: "
+					ret = error(_("could not write file: "
 						       "'%s'"), path);
 					goto unuse_commit_buffer;
 				}
+
+				ret = 0;
 			unuse_commit_buffer:
 				repo_unuse_commit_buffer(r, commit, p);
-				if (res) {
-					ret = res;
+				if (ret)
 					goto out;
-				}
 			}
 		}