diff mbox series

[Bug,debuginfod/27983] ignore duplicate urls

Message ID 308d7b3305efae40152abbf3b4c9c06b3d6a22fa.camel@klomp.org
State Committed
Headers show
Series [Bug,debuginfod/27983] ignore duplicate urls | expand

Commit Message

Mark Wielaard July 28, 2021, 2:55 p.m. UTC
Hi Noah,

On Mon, 2021-07-26 at 12:29 -0400, Noah Sanci via Elfutils-devel wrote:
> The realloc returning NULL issue has been resolved and the patch
> successfully rebased onto master. Please find these improvements
> attached.

This looks really good, but I found some small issues.

First it turns out reallocarray isn't available on some older systems.
This is easy to workaround though since it is a fairly simple function
we could provide ourselves if it isn't there. The attached patch does
that. I'll push it if it looks good.

Second there is a small memory leak found by valgrind. We only clean up
the server_url_list on failure, but we should also clean it up when we
are done on success:


Everything seems to pass everywhere.

run-debuginfod-find.sh is really convoluted and we really shouldn't add
more and more interacting tests to it. But if we do maybe we should use
the env DEBUGINFOD_URLS=... trick to minimize changing other tests in
the same file.

If you agree with the above two changes, could you resubmit the patch
with those?

Thanks,

Mark
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lib-Add-static-inline-reallocarray-fallback-function.patch
Type: text/x-patch
Size: 2251 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/elfutils-devel/attachments/20210728/eb6b369d/attachment.bin>

Comments

Noah Sanci July 28, 2021, 4:23 p.m. UTC | #1
Hello

This patch fixes a memory leak and slightly alters the PR27983 test,
isolating where its DEBUGINFO_URLS's duplicates are accessible, which
fixes a case of test failure on some systems.

Noah

On Wed, Jul 28, 2021 at 10:55 AM Mark Wielaard <mark@klomp.org> wrote:
>
> Hi Noah,
>
> On Mon, 2021-07-26 at 12:29 -0400, Noah Sanci via Elfutils-devel wrote:
> > The realloc returning NULL issue has been resolved and the patch
> > successfully rebased onto master. Please find these improvements
> > attached.
>
> This looks really good, but I found some small issues.
>
> First it turns out reallocarray isn't available on some older systems.
> This is easy to workaround though since it is a fairly simple function
> we could provide ourselves if it isn't there. The attached patch does
> that. I'll push it if it looks good.
>
> Second there is a small memory leak found by valgrind. We only clean up
> the server_url_list on failure, but we should also clean it up when we
> are done on success:
>
> diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-
> client.c
> index 9d049d33..0f65f115 100644
> --- a/debuginfod/debuginfod-client.c
> +++ b/debuginfod/debuginfod-client.c
> @@ -1191,6 +1191,9 @@ debuginfod_query_server (debuginfod_client *c,
>      }
>
>    free (data);
> +  for (int i = 0; i < num_urls; ++i)
> +    free(server_url_list[i]);
> +  free(server_url_list);
>    free (server_urls);
>
>    /* don't close fd - we're returning it */
>
> Finally on some systems, but not all, one of the tests in run-
> debuginfod-find.sh failed. In particular this one:
>
> # check out the debuginfod logs for the new style status lines
> # cat vlog$PORT2
> grep -q 'UA:.*XFF:.*GET /buildid/.* 200 ' vlog$PORT2
> grep -q 'UA:.*XFF:.*GET /metrics 200 ' vlog$PORT2
> grep -q 'UA:.*XFF:.*GET /badapi 503 ' vlog$PORT2
> grep -q 'UA:.*XFF:.*GET /buildid/deadbeef.* 404 ' vlog$PORT2
>
> That last one changed error message from 404 to 503. This seems related
> to the setting of DEBUGINFOD_URLS earlier by the new test you added. If
> I change that to:
>
> diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
> index c2c3b9c3..ec639a38 100755
> --- a/tests/run-debuginfod-find.sh
> +++ b/tests/run-debuginfod-find.sh
> @@ -367,8 +367,7 @@ wait_ready $PORT1 'found_sourcerefs_total{source=".rpm archive"}' $sourcefiles
>  ########################################################################
>  # PR27983 ensure no duplicate urls are used in when querying servers for files
>  rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
> -export DEBUGINFOD_URLS="http://127.0.0.1:$PORT1 http://127.0.0.1:$PORT1 http://127.0.0.1:$PORT1 http:127.0.0.1:7999"
> -env LD_LIBRARY_PATH=$ldpath ${abs_top_builddir}/debuginfod/debuginfod-find -v executable $BUILDID2 > vlog4 2>&1 || true
> +env DEBUGINFOD_URLS="http://127.0.0.1:$PORT1 http://127.0.0.1:$PORT1 http://127.0.0.1:$PORT1 http:127.0.0.1:7999" LD_LIBRARY_PATH=$ldpath ${abs_top_builddir}/debuginfod/debuginfod-find -v executable $BUILDID2 > vlog4 2>&1 || true
>  tempfiles vlog4
>  if [ $( grep -c 'duplicate url: http://127.0.0.1:'$PORT1'.*' vlog4 ) -ne 2 ]; then
>    echo "Duplicated servers remain";
>
> Everything seems to pass everywhere.
>
> run-debuginfod-find.sh is really convoluted and we really shouldn't add
> more and more interacting tests to it. But if we do maybe we should use
> the env DEBUGINFOD_URLS=... trick to minimize changing other tests in
> the same file.
>
> If you agree with the above two changes, could you resubmit the patch
> with those?
>
> Thanks,
>
> Mark
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-debuginfod-PR27983-ignore-duplicate-urls.patch
Type: text/x-patch
Size: 9879 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/elfutils-devel/attachments/20210728/2c47340a/attachment-0001.bin>
Mark Wielaard July 29, 2021, 12:20 p.m. UTC | #2
Hi Noah,

On Wed, 2021-07-28 at 12:23 -0400, Noah Sanci via Elfutils-devel wrote:
> This patch fixes a memory leak and slightly alters the PR27983 test,
> isolating where its DEBUGINFO_URLS's duplicates are accessible, which
> fixes a case of test failure on some systems.

Great. I pushed my patch for reallocarray support on older systems and
this one (with two small whitespace fixes).

Thanks,

Mark
diff mbox series

Patch

diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-
client.c
index 9d049d33..0f65f115 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -1191,6 +1191,9 @@  debuginfod_query_server (debuginfod_client *c,
     }
 
   free (data);
+  for (int i = 0; i < num_urls; ++i)
+    free(server_url_list[i]);
+  free(server_url_list);
   free (server_urls);
 
   /* don't close fd - we're returning it */

Finally on some systems, but not all, one of the tests in run-
debuginfod-find.sh failed. In particular this one:

# check out the debuginfod logs for the new style status lines
# cat vlog$PORT2
grep -q 'UA:.*XFF:.*GET /buildid/.* 200 ' vlog$PORT2
grep -q 'UA:.*XFF:.*GET /metrics 200 ' vlog$PORT2
grep -q 'UA:.*XFF:.*GET /badapi 503 ' vlog$PORT2
grep -q 'UA:.*XFF:.*GET /buildid/deadbeef.* 404 ' vlog$PORT2

That last one changed error message from 404 to 503. This seems related
to the setting of DEBUGINFOD_URLS earlier by the new test you added. If
I change that to:

diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index c2c3b9c3..ec639a38 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -367,8 +367,7 @@  wait_ready $PORT1 'found_sourcerefs_total{source=".rpm archive"}' $sourcefiles
 ########################################################################
 # PR27983 ensure no duplicate urls are used in when querying servers for files
 rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
-export DEBUGINFOD_URLS="http://127.0.0.1:$PORT1 http://127.0.0.1:$PORT1 http://127.0.0.1:$PORT1 http:127.0.0.1:7999"
-env LD_LIBRARY_PATH=$ldpath ${abs_top_builddir}/debuginfod/debuginfod-find -v executable $BUILDID2 > vlog4 2>&1 || true
+env DEBUGINFOD_URLS="http://127.0.0.1:$PORT1 http://127.0.0.1:$PORT1 http://127.0.0.1:$PORT1 http:127.0.0.1:7999" LD_LIBRARY_PATH=$ldpath ${abs_top_builddir}/debuginfod/debuginfod-find -v executable $BUILDID2 > vlog4 2>&1 || true
 tempfiles vlog4
 if [ $( grep -c 'duplicate url: http://127.0.0.1:'$PORT1'.*' vlog4 ) -ne 2 ]; then
   echo "Duplicated servers remain";