threads / patch / 51065

patchhttp-push: workaround for format overflow warning in gcc >= 9

Subject: [PATCH] http-push: workaround for format overflow warning in gcc >= 9

## tl;dr

3 messages between May 10, 2019 and May 13, 2019. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Carlo Marcelo Arenas Belón· May 10, 2019, 06:58 UTC · lore

In function 'finish_request',
    inlined from 'process_response' at http-push.c:248:2:
http-push.c:587:4: warning: '%s' directive argument is null [-Wformat-overflow=]
  587 |    fprintf(stderr, "Unable to get pack file %s\n%s",
      |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  588 |     request->url, curl_errorstr);
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
---
 http-push.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Show changes to http-push.c +2 −1

diff --git a/http-push.c b/http-push.c
index f675a96316..2db553ef38 100644
--- a/http-push.c
+++ b/http-push.c
@@ -585,7 +585,8 @@ static void finish_request(struct transfer_request *request)
 		int fail = 1;
 		if (request->curl_result != CURLE_OK) {
 			fprintf(stderr, "Unable to get pack file %s\n%s",
-				request->url, curl_errorstr);
+				request->url ? request->url : "",
+				curl_errorstr);
 		} else {
 			preq = (struct http_pack_request *)request->userData;

-- 
2.21.0

Eric Sunshine· May 10, 2019, 07:50 UTC · re: Carlo Marcelo Arenas Belón · lore

Re: [PATCH] http-push: workaround for format overflow warning in gcc >= 9

On Fri, May 10, 2019 at 2:59 AM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote:

Show 8 quoted lines

> In function 'finish_request',
>     inlined from 'process_response' at http-push.c:248:2:
> http-push.c:587:4: warning: '%s' directive argument is null [-Wformat-overflow=]
>   587 |    fprintf(stderr, "Unable to get pack file %s\n%s",
>       |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   588 |     request->url, curl_errorstr);
>       |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ---

Missing sign-off.

Show 9 quoted lines

> diff --git a/http-push.c b/http-push.c
> @@ -585,7 +585,8 @@ static void finish_request(struct transfer_request *request)
>                 int fail = 1;
>                 if (request->curl_result != CURLE_OK) {
>                         fprintf(stderr, "Unable to get pack file %s\n%s",
> -                               request->url, curl_errorstr);
> +                               request->url ? request->url : "",
> +                               curl_errorstr);
>                 } else {

If I'm reading the code correctly, the conditional and "true" branch of the ternary expression are dead code since 'request->url' will unconditionally be NULL due to the:

    /* URL is reused for MOVE after PUT */
    if (request->state != RUN_PUT) {
        FREE_AND_NULL(request->url);
    }

earlier in the function. If you want to present a meaningful error message here, I could imagine squirreling-away the URL so it can be used in the error message, or re-working the code so that FREE_AND_NULL(request->url) is only done when and if needed.

Junio C Hamano· May 13, 2019, 05:57 UTC · re: Eric Sunshine · lore

Re: [PATCH] http-push: workaround for format overflow warning in gcc >= 9

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 20 quoted lines

>>                 if (request->curl_result != CURLE_OK) {
>>                         fprintf(stderr, "Unable to get pack file %s\n%s",
>> -                               request->url, curl_errorstr);
>> +                               request->url ? request->url : "",
>> +                               curl_errorstr);
>>                 } else {
>
> If I'm reading the code correctly, the conditional and "true" branch
> of the ternary expression are dead code since 'request->url' will
> unconditionally be NULL due to the:
>
>     /* URL is reused for MOVE after PUT */
>     if (request->state != RUN_PUT) {
>         FREE_AND_NULL(request->url);
>     }
>
> earlier in the function. If you want to present a meaningful error
> message here, I could imagine squirreling-away the URL so it can be
> used in the error message, or re-working the code so that
> FREE_AND_NULL(request->url) is only done when and if needed.

That matches my understanding.