Download raw body.
ID metadata leak
Stefan Sperling <stsp@stsp.name> wrote:
> On Fri, Nov 08, 2024 at 10:35:34AM -0600, Kyle Ackerman wrote:
> > ping
>
> Committed now, thanks!
I think there's still a leak in got_send_pack() when the reference is a
duplicate and thus not inserted into the `have_refs` list in the call to
insert_sendable_ref(). Specifically, the `s` argument passed as the
target_refname parameter, which points to an object obtained by either
strdup(3) or asprintf(3), is leaked in the duplicate case. It is
unconditionally set to NULL on each iteration with the assumption it
will be released in the subsequent got_pathlist_free() call.
Similarly, got_fetch_pack() still leaks `refname`, which is also the
argument passed as the path parameter to got_pathlist_insert(). Like
`id`, it is allocated in got_privsep_recv_fetch_progress(), but it is
not released in the duplicate case like we now do with `id`. Relatedly,
whenever the got_pathlist_insert() call in got_fetch_pack() fails, the
path entry is not inserted into the refs list and we jump straight to
done:, so both refname and id are leaked.
The first of the below diffs plugs these leaks. The second is a small,
nearby whitespace fix spotted while investigating this.
-----------------------------------------------
commit ecd282adab9a71f73be6b57b17ea572b72179c17
from: Mark Jamsek <mark@jamsek.dev>
date: Tue Nov 12 15:38:29 2024 UTC
plug memory leaks in 'got fetch' and 'got send'
In addition to the previous plugged leaks here, free what would be
the path entry's path member, which is leaked when attempting to
add a duplicate. And in the fetch case, free refname and id when
got_pathlist_insert() fails.
M include/got_error.h | 1+ 0-
M lib/error.c | 1+ 0-
M lib/fetch.c | 5+ 3-
M lib/send.c | 16+ 5-
4 files changed, 23 insertions(+), 8 deletions(-)
diff d3e5e1573bd462d20d38aa0aa6535fcea6c0a948 ecd282adab9a71f73be6b57b17ea572b72179c17
commit - d3e5e1573bd462d20d38aa0aa6535fcea6c0a948
commit + ecd282adab9a71f73be6b57b17ea572b72179c17
blob - cc02089bfcb70faf1775da8b8a38eb7bfd903018
blob + 4181bc79115b549c541be0a74f00f51030a6135d
--- include/got_error.h
+++ include/got_error.h
@@ -188,6 +188,7 @@
#define GOT_ERR_BUNDLE_FORMAT 171
#define GOT_ERR_BAD_KEYWORD 172
#define GOT_ERR_UNKNOWN_CAPA 173
+#define GOT_ERR_REF_DUP_ENTRY 174
struct got_error {
int code;
blob - 8872bef06f42c654a7f90d728cc11aadb624cf2e
blob + b75dde1c3fc3d4e86f60fe5fa47a7f09526358bf
--- lib/error.c
+++ lib/error.c
@@ -239,6 +239,7 @@ static const struct got_error got_errors[] = {
{ GOT_ERR_BUNDLE_FORMAT, "unknown git bundle version" },
{ GOT_ERR_BAD_KEYWORD, "invalid commit keyword" },
{ GOT_ERR_UNKNOWN_CAPA, "unknown capability" },
+ { GOT_ERR_REF_DUP_ENTRY, "duplicate reference entry" },
};
static struct got_custom_error {
blob - f03b63655d11f0912e5c2dd4b182296979de8e36
blob + 4d56359502a5b5c1e7fc70c62f2355e4e13e7960
--- lib/fetch.c
+++ lib/fetch.c
@@ -314,10 +314,12 @@ got_fetch_pack(struct got_object_id **pack_hash, struc
packfile_size_cur = 0;
if (!done && refname && id) {
err = got_pathlist_insert(&pe, refs, refname, id);
- if (err)
- goto done;
- if (pe == NULL)
+ if (err || pe == NULL) {
free(id);
+ free(refname);
+ if (err != NULL)
+ goto done;
+ }
} else if (!done && server_progress) {
char *p;
/*
blob - 8ec7b9fb1ba48a9fa452a728bb1787df461ef284
blob + 65890785ca1e642d75f9495fc8450e57bdc2806a
--- lib/send.c
+++ lib/send.c
@@ -202,10 +202,13 @@ insert_sendable_ref(struct got_pathlist_head *refs, co
}
err = got_pathlist_insert(&new, refs, target_refname, id);
+ if (new == NULL && err == NULL)
+ err = got_error(GOT_ERR_REF_DUP_ENTRY);
+
done:
if (ref)
got_ref_close(ref);
- if (err || new == NULL /* duplicate */)
+ if (err)
free(id);
return err;
}
@@ -388,8 +391,12 @@ got_send_pack(const char *remote_name, struct got_path
}
}
err = insert_sendable_ref(&have_refs, branchname, s, repo);
- if (err)
- goto done;
+ if (err) {
+ if (err->code != GOT_ERR_REF_DUP_ENTRY)
+ goto done;
+ err = NULL;
+ free(s);
+ }
s = NULL;
}
@@ -424,8 +431,12 @@ got_send_pack(const char *remote_name, struct got_path
}
}
err = insert_sendable_ref(&have_refs, s, s, repo);
- if (err)
- goto done;
+ if (err) {
+ if (err->code != GOT_ERR_REF_DUP_ENTRY)
+ goto done;
+ err = NULL;
+ free(s);
+ }
s = NULL;
}
-----------------------------------------------
commit e4716d841dc7bebea2ceb5cd9da6807682c8cec3 (main)
from: Mark Jamsek <mark@jamsek.dev>
date: Tue Nov 12 15:38:30 2024 UTC
whitespace
M lib/send.c | 1+ 1-
1 file changed, 1 insertion(+), 1 deletion(-)
diff ecd282adab9a71f73be6b57b17ea572b72179c17 e4716d841dc7bebea2ceb5cd9da6807682c8cec3
commit - ecd282adab9a71f73be6b57b17ea572b72179c17
commit + e4716d841dc7bebea2ceb5cd9da6807682c8cec3
blob - 65890785ca1e642d75f9495fc8450e57bdc2806a
blob + 4fb5f3aad871abb2d8ad18e445fb9dff32c8c6b0
--- lib/send.c
+++ lib/send.c
@@ -196,7 +196,7 @@ insert_sendable_ref(struct got_pathlist_head *refs, co
case GOT_OBJ_TYPE_TAG:
break;
default:
- err = got_error_fmt(GOT_ERR_OBJ_TYPE," cannot send %s",
+ err = got_error_fmt(GOT_ERR_OBJ_TYPE, "cannot send %s",
refname);
goto done;
}
--
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68
ID metadata leak