From patchwork Wed Jul 28 14:55:00 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Wielaard X-Patchwork-Id: 45840 From: mark@klomp.org (Mark Wielaard) Date: Wed, 28 Jul 2021 16:55:00 +0200 Subject: [Bug debuginfod/27983] ignore duplicate urls In-Reply-To: References: <7b021d11929e451d77961ab183cd97f3329a6dce.camel@klomp.org> Message-ID: <308d7b3305efae40152abbf3b4c9c06b3d6a22fa.camel@klomp.org> 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: 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";