Hello devs, I have spotted and repaired a memory leak in iri.c (idn_encode). The problem was, that `remote_to_utf8` would allocate a new buffer, but the buffer was never freed. (It was the `new` pointer, later copied to `host`, used for `idna_to_ascii_8z`. After returning from idn_encode, it was out of scope.) To avoid similar problems in the future, I have also given more meaningful names to the pointers, and added a new pointer to get rid of the confusing variable reusage. Please review the patch, and if you agree, please apply.
Also, IMHO it is worth to add a suppression file for valgrind tests in wget. Otherwise, the tests do not pass. (Apart from the bug mentioned above, there is a Valgrind's false positive at `idna_to_ascii_4z` in `libidn.so`.) And since the first part (`tests`) fails, `make check` does not even make it to the second part (`testenv`). (Which is probably another bug, not a feature :D.) The problem is, that I do not see a simple way to add it in one place; as Valgrind invocation is handled separately in both `testenv` and `tests`. However, the problem appears only in `tests` currently, therefore maybe it could be added just there. I am attaching my workaround for the problem (Valgrind's suppression file and a patch to WgetTests.pm) - it could be probably done in a more elegant way. Regards, Hubert
From 0af276838c7400c9ff8bb936597741effbaf13a4 Mon Sep 17 00:00:00 2001 From: Hubert Tarasiuk <[email protected]> Date: Mon, 6 Apr 2015 12:00:39 +0200 Subject: [PATCH 1/2] Remove memory leak in idn_encode * src/iri.c (idn_encode): The buffer allocated by remote_to_utf8 was never freed. --- src/iri.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/iri.c b/src/iri.c index dc1ebd0..b5e0a9e 100644 --- a/src/iri.c +++ b/src/iri.c @@ -225,19 +225,25 @@ locale_to_utf8 (const char *str) char * idn_encode (struct iri *i, char *host) { - char *new; + char *utf8_encoded; + char *ascii_encoded; int ret; /* Encode to UTF-8 if not done */ if (!i->utf8_encode) { - if (!remote_to_utf8 (i, (const char *) host, (const char **) &new)) + if (!remote_to_utf8 (i, (const char *) host, (const char **) &utf8_encoded)) return NULL; /* Nothing to encode or an error occured */ - host = new; } + else + utf8_encoded = host; /* toASCII UTF-8 NULL terminated string */ - ret = idna_to_ascii_8z (host, &new, IDNA_FLAGS); + ret = idna_to_ascii_8z (utf8_encoded, &ascii_encoded, IDNA_FLAGS); + + if (utf8_encoded != host) + xfree (utf8_encoded); + if (ret != IDNA_SUCCESS) { /* sXXXav : free new when needed ! */ @@ -246,7 +252,7 @@ idn_encode (struct iri *i, char *host) return NULL; } - return new; + return ascii_encoded; } /* Try to decode an "ASCII encoded" host. Return the new domain in the locale -- 2.3.5
From 24fca55af1396ddeb2351555a1222ef83156e2c2 Mon Sep 17 00:00:00 2001 From: Hubert Tarasiuk <[email protected]> Date: Mon, 6 Apr 2015 12:33:37 +0200 Subject: [PATCH 2/2] Add Valgrind suppresion for libidn.so at idna_to_ascii_4z. * tests/WgetTests.pm (run): Include suppression file when running Valgrind. * tests/valgrind-suppressions: Add suppression for idn_to_ascii_4z. --- tests/WgetTests.pm | 2 +- tests/valgrind-suppressions | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 tests/valgrind-suppressions diff --git a/tests/WgetTests.pm b/tests/WgetTests.pm index 3d3b9dd..5dd5214 100644 --- a/tests/WgetTests.pm +++ b/tests/WgetTests.pm @@ -122,7 +122,7 @@ sub run elsif ($valgrind == 1) { $cmdline = - 'valgrind --error-exitcode=301 --leak-check=yes --track-origins=yes ' + 'valgrind --suppressions=../../valgrind-suppressions --error-exitcode=301 --leak-check=yes --track-origins=yes ' . $cmdline; } else diff --git a/tests/valgrind-suppressions b/tests/valgrind-suppressions new file mode 100644 index 0000000..160e234 --- /dev/null +++ b/tests/valgrind-suppressions @@ -0,0 +1,8 @@ +{ + False positive in libidn.so. + Memcheck:Addr4 + fun:idna_to_ascii_4z + fun:idna_to_ascii_8z + fun:idn_encode + fun:url_parse +} -- 2.3.5
signature.asc
Description: OpenPGP digital signature
