From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (wildebeest.demon.nl [212.238.236.112]) by sourceware.org (Postfix) with ESMTPS id 095F73858414 for ; Wed, 8 Sep 2021 13:38:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 095F73858414 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: from tarox.wildebeest.org (83-87-18-245.cable.dynamic.v4.ziggo.nl [83.87.18.245]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 8B59E301FE93; Wed, 8 Sep 2021 15:38:44 +0200 (CEST) Received: by tarox.wildebeest.org (Postfix, from userid 1000) id 798714D40ACD; Wed, 8 Sep 2021 15:38:43 +0200 (CEST) Message-ID: Subject: Re: [Bug debuginfod/28034] client-side %-escape url characters From: Mark Wielaard To: Noah Sanci , "Frank Ch. Eigler" Cc: elfutils-devel@sourceware.org Date: Wed, 08 Sep 2021 15:38:43 +0200 In-Reply-To: References: <20210826210213.GM416@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Evolution 3.28.5 (3.28.5-10.el7) Mime-Version: 1.0 X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Elfutils-devel mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Sep 2021 13:38:48 -0000 Hi Noah, This looks good, but the description is not really what the patch does. It is part of the PR28034 fix, but repeating that problem statement doesn't really make clear what this patch is about. Could explain in the commit message that curl_easy_escape () also escapes '/' to '%2F' and that we want to reverse that back to '/'? Also some (tiny) optimization hints. This can be done outside/before the loop /* Initialize each handle. */ for (int i =3D 0; i < num_urls; i++) So you only need to escape once. You of course then need to make sure the escaped_string is freed after the loop. We already check that the first char is a '/'. It seems silly to curl escape that one and then unescape it again. So maybe curl_easy_escape (data[i].handle, filename + 1, 0) and then change the snprintf pattern to "%s/%s/%s/%s"? ^ the slash got readded here. The strlen inside the while loop can also be done outside and then calculated instead of running strlen on the tail every time. size_t escaped_strlen =3D strlen (escaped_string); while( (loc =3D strstr(loc, "%2F")) ) { loc[0] =3D '/'; // pull the string back after replacement escaped_strlen -=3D (loc + 3) - escaped_string); memmove(loc+1, loc+3, escaped_strlen + 1); } (Note, not even compiled, so I might be wrong) Lastly I assume there are already testcases that cover this functionality? Just wanting to know how you tested it. Thanks, Mark