Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix some memory leaks in the NBD server and enable asan for the tests #1374

Merged
merged 7 commits into from Apr 4, 2024
Merged

fix some memory leaks in the NBD server and enable asan for the tests #1374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ff3836b
src/nbd: fix memory leak of GVariantDict contents
jluebbe Mar 28, 2024
aecc3ad
src/nbd: fix leaks of configuration strings in the NBD server
jluebbe Mar 28, 2024
72c811a
src/nbd: fix string leak
jluebbe Mar 28, 2024
fc777f6
src/nbd: fix leak of disconnect request
jluebbe Mar 28, 2024
a95ce8f
src/nbd: fix memory leak on error shutdown
jluebbe Mar 28, 2024
68ebdd0
src/nbd: fix leak of headers GStrv
jluebbe Mar 28, 2024
372fce7
.github/workflows/sanitizers: enable tests for nbd
jluebbe Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/sanitizers.yml
View file
Open in desktop
Expand Up @@ -41,7 +41,7 @@ jobs:
build/test/hash_index-test
build/test/install-test
build/test/manifest-test
build/test/nbd-test -l
build/test/nbd-test
build/test/network-test
build/test/progress-test
build/test/service-test -l
Expand Down
32 changes: 19 additions & 13 deletions src/nbd.c
View file
Open in desktop
Expand Up @@ -87,6 +87,7 @@
g_free(nbd_srv->tls_ca);
g_strfreev(nbd_srv->headers);
g_strfreev(nbd_srv->info_headers);
g_free(nbd_srv->effective_url);
g_free(nbd_srv);
}

Expand Down Expand Up @@ -286,13 +287,12 @@
gchar *tls_key; /* local file or PKCS#11 URI */
gchar *tls_ca; /* local file */
gboolean tls_no_verify;
GStrv headers; /* array of strings such as 'Foo: bar' */
struct curl_slist *headers_slist;
struct curl_slist *initial_headers_slist;

/* runtime state */
CURLM *multi;
gboolean done;
struct curl_slist *headers_slist;
struct curl_slist *initial_headers_slist;

/* statistics */
RaucStats *dl_size, *dl_speed, *namelookup, *connect, *starttransfer, *total;
Expand Down Expand Up @@ -526,7 +526,7 @@
g_error("unexpected error from curl_multi_add_handle in %s", G_STRFUNC);
}

/* Appends Gstrv elements to curl_slist (only pointers, no data).
/* Appends Gstrv elements to curl_slist (strings are copied).
* If curl_slist does not exist yet (NULL passed), it will be created.
* The created list needs to be freed (after usage) by the caller with
* curl_slist_free_all(). */
Expand Down Expand Up @@ -556,13 +556,14 @@
gboolean res = FALSE;
CURLcode code = 0;
CURLMcode mcode = 0;
g_autofree guint8 *data = g_malloc(xfer->request.len);
g_autoptr(GVariant) v = NULL;
GVariantDict dict;

/* only read from the client on the first try */
if (!ctx->url) {
GStrv info_headers = NULL; /* array of strings such as 'Foo: bar' */
g_autofree guint8 *data = g_malloc(xfer->request.len);
g_autoptr(GVariant) v = NULL;
g_auto(GVariantDict) dict = G_VARIANT_DICT_INIT(NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe note that this changes from stack-allocated to heap-allocated and that the former variant lacked the g_variant_dict_clear() call?

Also, the commit contains an unrelated newline change and unrelated code move. But since the result is still clear enough to read I'd be fine with that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still stack-allocated, which is what g_auto is used for.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouh, right. My fault.

g_auto(GStrv) headers = NULL; /* array of strings such as 'Foo: bar' */
g_auto(GStrv) info_headers = NULL; /* array of strings such as 'Foo: bar' */

Check warning on line 566 in src/nbd.c

View check run for this annotation

Codecov / codecov/patch

src/nbd.c#L562-L566 

Added lines #L562 - L566 were not covered by tests

res = r_read_exact(ctx->sock, (guint8*)data, xfer->request.len, NULL);
g_assert_true(res);
Expand All @@ -578,19 +579,18 @@
}

g_variant_dict_init(&dict, v);

g_variant_dict_lookup(&dict, "url", "s", &ctx->url);
g_variant_dict_lookup(&dict, "cert", "s", &ctx->tls_cert);
g_variant_dict_lookup(&dict, "key", "s", &ctx->tls_key);
g_variant_dict_lookup(&dict, "ca", "s", &ctx->tls_ca);
g_variant_dict_lookup(&dict, "no-verify", "b", &ctx->tls_no_verify);
g_variant_dict_lookup(&dict, "headers", "^as", &ctx->headers);
g_variant_dict_lookup(&dict, "headers", "^as", &headers);

Check warning on line 587 in src/nbd.c

View check run for this annotation

Codecov / codecov/patch

src/nbd.c#L587 

Added line #L587 was not covered by tests
g_variant_dict_lookup(&dict, "info-headers", "^as", &info_headers);
g_assert_nonnull(ctx->url);

if (ctx->headers) {
ctx->headers_slist = gstrv_add_to_slist(NULL, ctx->headers);
ctx->initial_headers_slist = gstrv_add_to_slist(NULL, ctx->headers);
if (headers) {
ctx->headers_slist = gstrv_add_to_slist(NULL, headers);
ctx->initial_headers_slist = gstrv_add_to_slist(NULL, headers);

Check warning on line 593 in src/nbd.c

View check run for this annotation

Codecov / codecov/patch

src/nbd.c#L591-L593 

Added lines #L591 - L593 were not covered by tests
}
if (info_headers) {
ctx->initial_headers_slist = gstrv_add_to_slist(ctx->initial_headers_slist, info_headers);
Expand Down Expand Up @@ -636,6 +636,7 @@
case NBD_CMD_DISC: {
g_message("nbd server received disconnect request");
ctx->done = TRUE;
g_free(xfer); /* not queued via curl_multi_add_handle */

Check warning on line 639 in src/nbd.c

View check run for this annotation

Codecov / codecov/patch

src/nbd.c#L639 

Added line #L639 was not covered by tests
break;
}
case RAUC_NBD_CMD_CONFIGURE: {
Expand Down Expand Up @@ -930,6 +931,7 @@
error,
R_NBD_ERROR, R_NBD_ERROR_SHUTDOWN,
"finish_request failed, shutting down");
g_free(xfer);

Check warning on line 934 in src/nbd.c

View check run for this annotation

Codecov / codecov/patch

src/nbd.c#L934 

Added line #L934 was not covered by tests
goto out;
}

Expand All @@ -952,6 +954,10 @@

res = TRUE;
out:
g_clear_pointer(&ctx.url, g_free);
jluebbe marked this conversation as resolved.
Show resolved Hide resolved
g_clear_pointer(&ctx.tls_cert, g_free);
g_clear_pointer(&ctx.tls_key, g_free);
g_clear_pointer(&ctx.tls_ca, g_free);

Check warning on line 960 in src/nbd.c

View check run for this annotation

Codecov / codecov/patch

src/nbd.c#L957-L960 

Added lines #L957 - L960 were not covered by tests
g_clear_pointer(&ctx.dl_size, r_stats_free);
g_clear_pointer(&ctx.dl_speed, r_stats_free);
g_clear_pointer(&ctx.namelookup, r_stats_free);
Expand Down