diff mbox series

[10/11] config: fix a leak in git_config_copy_or_rename_section_in_file

Message ID 3ee93720-dce0-8f90-68bf-0242e5731a36@gmail.com (mailing list archive)
State Accepted
Commit 5e786ed3ee109f166b05916f3f5410f76164f8db
Headers show
Series [01/11] rev-parse: fix a leak with --abbrev-ref | expand

Commit Message

Rubén Justo June 11, 2023, 6:50 p.m. UTC 
A branch can have its configuration spread over several configuration
sections.  This situation was already foreseen in 52d59cc645 (branch:
add a --copy (-c) option to go with --move (-m), 2017-06-18) when
"branch -c" was introduced.

Unfortunately, a leak was also introduced:

   $ git branch foo
   $ cat >> .git/config <<EOF
   [branch "foo"]
   	some-key-a = a value
   [branch "foo"]
   	some-key-b = b value
   [branch "foo"]
   	some-key-c = c value
   EOF
   $ git branch -c foo bar

   Direct leak of 130 byte(s) in 2 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in strbuf_grow strbuf.c
       ... in strbuf_vaddf strbuf.c
       ... in strbuf_addf strbuf.c
       ... in store_create_section config.c
       ... in git_config_copy_or_rename_section_in_file config.c
       ... in git_config_copy_section_in_file config.c
       ... in git_config_copy_section config.c
       ... in copy_or_rename_branch builtin/branch.c
       ... in cmd_branch builtin/branch.c
       ... in run_builtin git.c

Let's fix it.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 config.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Jeff King June 12, 2023, 4:05 a.m. UTC | #1 
On Sun, Jun 11, 2023 at 08:50:45PM +0200, Rubén Justo wrote:

> diff --git a/config.c b/config.c
> index 39a7d7422c..207e4394a3 100644
> --- a/config.c
> +++ b/config.c
> @@ -3833,6 +3833,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
>  						output[0] = '\t';
>  					}
>  				} else {
> +					strbuf_release(&copystr);
>  					copystr = store_create_section(new_name, &store);
>  				}
>  			}

Wow, I did a double-take on this code. It is uncommon in our codebase to
assign a struct by value in a function return like this, and doubly
weird to assign a strbuf (since the whole point of strbuf is to use the
opaque functions to make sure we aren't overwriting existing allocations
or aliasing pointers).

I think your fix here is the correct thing if we aren't going to clean
up the code further.

The more usual thing for our codebase would be refactoring like:

diff --git a/config.c b/config.c
index b79baf83e3..f5a7cced7c 100644
--- a/config.c
+++ b/config.c
@@ -3140,37 +3140,36 @@ static int write_error(const char *filename)
 	return 4;
 }
 
-static struct strbuf store_create_section(const char *key,
-					  const struct config_store_data *store)
+static void store_create_section(const char *key,
+				 const struct config_store_data *store,
+				 struct strbuf *sb)
 {
 	const char *dot;
 	size_t i;
-	struct strbuf sb = STRBUF_INIT;
 
 	dot = memchr(key, '.', store->baselen);
 	if (dot) {
-		strbuf_addf(&sb, "[%.*s \"", (int)(dot - key), key);
+		strbuf_addf(sb, "[%.*s \"", (int)(dot - key), key);
 		for (i = dot - key + 1; i < store->baselen; i++) {
 			if (key[i] == '"' || key[i] == '\\')
-				strbuf_addch(&sb, '\\');
-			strbuf_addch(&sb, key[i]);
+				strbuf_addch(sb, '\\');
+			strbuf_addch(sb, key[i]);
 		}
-		strbuf_addstr(&sb, "\"]\n");
+		strbuf_addstr(sb, "\"]\n");
 	} else {
-		strbuf_addch(&sb, '[');
-		strbuf_add(&sb, key, store->baselen);
-		strbuf_addstr(&sb, "]\n");
+		strbuf_addch(sb, '[');
+		strbuf_add(sb, key, store->baselen);
+		strbuf_addstr(sb, "]\n");
 	}
-
-	return sb;
 }
 
 static ssize_t write_section(int fd, const char *key,
 			     const struct config_store_data *store)
 {
-	struct strbuf sb = store_create_section(key, store);
+	struct strbuf sb = STRBUF_INIT;
 	ssize_t ret;
 
+	store_create_section(key, store, &sb);
 	ret = write_in_full(fd, sb.buf, sb.len);
 	strbuf_release(&sb);
 
@@ -3833,7 +3832,9 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
 						output[0] = '\t';
 					}
 				} else {
-					copystr = store_create_section(new_name, &store);
+					strbuf_reset(&copystr);
+					store_create_section(new_name, &store,
+							     &copystr);
 				}
 			}
 			remove = 0;
Rubén Justo June 16, 2023, 11:04 p.m. UTC | #2 
On 12-jun-2023 00:05:18, Jeff King wrote:
> On Sun, Jun 11, 2023 at 08:50:45PM +0200, Rubén Justo wrote:
> 
> > diff --git a/config.c b/config.c
> > index 39a7d7422c..207e4394a3 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -3833,6 +3833,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
> >  						output[0] = '\t';
> >  					}
> >  				} else {
> > +					strbuf_release(&copystr);
> >  					copystr = store_create_section(new_name, &store);
> >  				}
> >  			}
> 
> Wow, I did a double-take on this code. It is uncommon in our codebase to
> assign a struct by value in a function return like this, and doubly
> weird to assign a strbuf (since the whole point of strbuf is to use the
> opaque functions to make sure we aren't overwriting existing allocations
> or aliasing pointers).
> 
> I think your fix here is the correct thing if we aren't going to clean
> up the code further.
> 
> The more usual thing for our codebase would be refactoring like:
> 
> diff --git a/config.c b/config.c
> index b79baf83e3..f5a7cced7c 100644
> --- a/config.c
> +++ b/config.c
> @@ -3140,37 +3140,36 @@ static int write_error(const char *filename)
>  	return 4;
>  }
>  
> -static struct strbuf store_create_section(const char *key,
> -					  const struct config_store_data *store)
> +static void store_create_section(const char *key,
> +				 const struct config_store_data *store,
> +				 struct strbuf *sb)
>  {
>  	const char *dot;
>  	size_t i;
> -	struct strbuf sb = STRBUF_INIT;
>  
>  	dot = memchr(key, '.', store->baselen);
>  	if (dot) {
> -		strbuf_addf(&sb, "[%.*s \"", (int)(dot - key), key);
> +		strbuf_addf(sb, "[%.*s \"", (int)(dot - key), key);
>  		for (i = dot - key + 1; i < store->baselen; i++) {
>  			if (key[i] == '"' || key[i] == '\\')
> -				strbuf_addch(&sb, '\\');
> -			strbuf_addch(&sb, key[i]);
> +				strbuf_addch(sb, '\\');
> +			strbuf_addch(sb, key[i]);
>  		}
> -		strbuf_addstr(&sb, "\"]\n");
> +		strbuf_addstr(sb, "\"]\n");
>  	} else {
> -		strbuf_addch(&sb, '[');
> -		strbuf_add(&sb, key, store->baselen);
> -		strbuf_addstr(&sb, "]\n");
> +		strbuf_addch(sb, '[');
> +		strbuf_add(sb, key, store->baselen);
> +		strbuf_addstr(sb, "]\n");
>  	}
> -
> -	return sb;
>  }
>  
>  static ssize_t write_section(int fd, const char *key,
>  			     const struct config_store_data *store)
>  {
> -	struct strbuf sb = store_create_section(key, store);
> +	struct strbuf sb = STRBUF_INIT;
>  	ssize_t ret;
>  
> +	store_create_section(key, store, &sb);
>  	ret = write_in_full(fd, sb.buf, sb.len);
>  	strbuf_release(&sb);
>  
> @@ -3833,7 +3832,9 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
>  						output[0] = '\t';
>  					}
>  				} else {
> -					copystr = store_create_section(new_name, &store);
> +					strbuf_reset(&copystr);
> +					store_create_section(new_name, &store,
> +							     &copystr);
>  				}
>  			}
>  			remove = 0;

I have a draft with the exact same change.  I chose the simple fix,
though.  I wasn't expecting your review :)

I'll definitely re-roll with this.

Thanks!
diff mbox series

Patch

diff --git a/config.c b/config.c
index 39a7d7422c..207e4394a3 100644
--- a/config.c
+++ b/config.c
@@ -3833,6 +3833,7 @@  static int git_config_copy_or_rename_section_in_file(const char *config_filename
 						output[0] = '\t';
 					}
 				} else {
+					strbuf_release(&copystr);
 					copystr = store_create_section(new_name, &store);
 				}
 			}