[RFC/PATCH] imap-send: Code correctness flagged by clang

[RFC/PATCH] imap-send: Code correctness flagged by clang

[Ævar Arnfjörð Bjarmason]

Clang 1.1 flagged the following issues in imap-send.c, this change
fixes the warnings by moving some code around:

    imap-send.c:548:27: warning: data argument not used by format string [-Wformat-extra-args]
                               cmd->tag, cmd->cmd, cmd->cb.dlen);
                                                   ^

Here the sprintf format didn't use the cmd->cb.dlen argument if
cmd->cb.data was false. Change the code to use a if/else instead of a
two-level ternary to work it. This code was introduced with imap-send
itself in f2561fda.

    imap-send.c:1089:41: warning: conversion specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
                    snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
                                                        ~~^   ~~~~~~~~~~

Here sprintf is being given an int with a %hu format. Cast the
srvc->port to unsigned short to work it. This code was introduced in
94ad2437 to add IPv6 support.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 imap-send.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 1a577a0..4b25375 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -543,9 +543,14 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,
 	while (imap->literal_pending)
 		get_cmd_result(ctx, NULL);
 
-	bufl = nfsnprintf(buf, sizeof(buf), cmd->cb.data ? CAP(LITERALPLUS) ?
-			   "%d %s{%d+}\r\n" : "%d %s{%d}\r\n" : "%d %s\r\n",
-			   cmd->tag, cmd->cmd, cmd->cb.dlen);
+	if (cmd->cb.data) {
+		bufl = nfsnprintf(buf, sizeof(buf),
+		                  CAP(LITERALPLUS) ? "%d %s{%d+}\r\n" : "%d %s{%d}\r\n",
+		                  cmd->tag, cmd->cmd, cmd->cb.dlen);
+	} else {
+		bufl = nfsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag, cmd->cmd);
+	}
+
 	if (Verbose) {
 		if (imap->num_in_progress)
 			printf("(%d in progress) ", imap->num_in_progress);
@@ -1086,7 +1091,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 		int gai;
 		char portstr[6];
 
-		snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
+		snprintf(portstr, sizeof(portstr), "%hu", (unsigned short)srvc->port);
 
 		memset(&hints, 0, sizeof(hints));
 		hints.ai_socktype = SOCK_STREAM;
-- 
1.7.1

Re: [RFC/PATCH] imap-send: Code correctness flagged by clang

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> [Subject: imap-send: Code correctness flagged by clang]
>
> Clang 1.1 flagged the following issues in imap-send.c, this change
> fixes the warnings by moving some code around:
> 
>     imap-send.c:548:27: warning: data argument not used by format string [-Wformat-extra-args]
>                                cmd->tag, cmd->cmd, cmd->cb.dlen);
>                                                    ^
> 
> Here the sprintf format didn't use the cmd->cb.dlen argument if
> cmd->cb.data was false. Change the code to use a if/else instead of a
> two-level ternary to work it. This code was introduced with imap-send
> itself in f2561fda.
> 
>     imap-send.c:1089:41: warning: conversion specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
>                     snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
>                                                         ~~^   ~~~~~~~~~~
> 
> Here sprintf is being given an int with a %hu format. Cast the
> srvc->port to unsigned short to work it. This code was introduced in
> 94ad2437 to add IPv6 support.

Nitpick: that this was found by clang is probably not the first thing
a person trying to figure out what the patch does needs to know.
Maybe:

	Subject: imap-send: Fix sprintf usage

	When composing a command for the imap server, imap-send
	uses a single nfsnprintf() invocation for brevity
	instead of dealing separately with the case when there
	is a message to be sent and the case when there isn’t.
	The unused argument in the second case, while valid,
	is confusing for static analyzers and human readers.

	v1.6.4-rc0~117 (imap-send: add support for IPv6, 2009-05-25)
	mistakenly used %hu as the format for an int “port”, by
	analogy with existing usage for the unsigned short
	“addr.sin_port”.  Use %d instead.

	Noticed with clang.

> +++ b/imap-send.c
> @@ -543,9 +543,14 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,
>  	while (imap->literal_pending)
>  		get_cmd_result(ctx, NULL);
>  
> -	bufl = nfsnprintf(buf, sizeof(buf), cmd->cb.data ? CAP(LITERALPLUS) ?
> -			   "%d %s{%d+}\r\n" : "%d %s{%d}\r\n" : "%d %s\r\n",
> -			   cmd->tag, cmd->cmd, cmd->cb.dlen);
> +	if (cmd->cb.data) {
> +		bufl = nfsnprintf(buf, sizeof(buf),
> +		                  CAP(LITERALPLUS) ? "%d %s{%d+}\r\n" : "%d %s{%d}\r\n",
> +		                  cmd->tag, cmd->cmd, cmd->cb.dlen);
> +	} else {
> +		bufl = nfsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag, cmd->cmd);
> +	}
> +

Hmm, maybe this would be easier to read:

	if (!cmd->cb.data)
		bufl = nfsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag, cmd->cmd);
	else
		bufl = nfsnprintf(buf, sizeof(buf), "%d %s{%d%s}\r\n",
		                  cmd->tag, cmd->cmd, cmd->cb.dlen,
		                  CAP(LITERALPLUS) ? "+" : "");

i.e., putting the easier case first and avoiding a variable format string.

> @@ -1086,7 +1091,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
>  		int gai;
>  		char portstr[6];
>  
> -		snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
> +		snprintf(portstr, sizeof(portstr), "%hu", (unsigned short)srvc->port);

Why not

		snprintf(portstr, sizeof(portstr), "%d", srvc->port);

?

Thanks for checking the code.

Re: [RFC/PATCH] imap-send: Code correctness flagged by clang

[Ævar Arnfjörð Bjarmason]

On Sat, Aug 7, 2010 at 21:04, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> [Subject: imap-send: Code correctness flagged by clang]
>>
>> Clang 1.1 flagged the following issues in imap-send.c, this change
>> fixes the warnings by moving some code around:
>>
>>     imap-send.c:548:27: warning: data argument not used by format string [-Wformat-extra-args]
>>                                cmd->tag, cmd->cmd, cmd->cb.dlen);
>>                                                    ^
>>
>> Here the sprintf format didn't use the cmd->cb.dlen argument if
>> cmd->cb.data was false. Change the code to use a if/else instead of a
>> two-level ternary to work it. This code was introduced with imap-send
>> itself in f2561fda.
>>
>>     imap-send.c:1089:41: warning: conversion specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
>>                     snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
>>                                                         ~~^   ~~~~~~~~~~
>>
>> Here sprintf is being given an int with a %hu format. Cast the
>> srvc->port to unsigned short to work it. This code was introduced in
>> 94ad2437 to add IPv6 support.
>
> Nitpick: that this was found by clang is probably not the first thing
> a person trying to figure out what the patch does needs to know.
> Maybe:
>
>        Subject: imap-send: Fix sprintf usage
>
>        When composing a command for the imap server, imap-send
>        uses a single nfsnprintf() invocation for brevity
>        instead of dealing separately with the case when there
>        is a message to be sent and the case when there isn’t.
>        The unused argument in the second case, while valid,
>        is confusing for static analyzers and human readers.
>
>        v1.6.4-rc0~117 (imap-send: add support for IPv6, 2009-05-25)
>        mistakenly used %hu as the format for an int “port”, by
>        analogy with existing usage for the unsigned short
>        “addr.sin_port”.  Use %d instead.
>
>        Noticed with clang.

That looks better.

>> +++ b/imap-send.c
>> @@ -543,9 +543,14 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,
>>       while (imap->literal_pending)
>>               get_cmd_result(ctx, NULL);
>>
>> -     bufl = nfsnprintf(buf, sizeof(buf), cmd->cb.data ? CAP(LITERALPLUS) ?
>> -                        "%d %s{%d+}\r\n" : "%d %s{%d}\r\n" : "%d %s\r\n",
>> -                        cmd->tag, cmd->cmd, cmd->cb.dlen);
>> +     if (cmd->cb.data) {
>> +             bufl = nfsnprintf(buf, sizeof(buf),
>> +                               CAP(LITERALPLUS) ? "%d %s{%d+}\r\n" : "%d %s{%d}\r\n",
>> +                               cmd->tag, cmd->cmd, cmd->cb.dlen);
>> +     } else {
>> +             bufl = nfsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag, cmd->cmd);
>> +     }
>> +
>
> Hmm, maybe this would be easier to read:
>
>        if (!cmd->cb.data)
>                bufl = nfsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag, cmd->cmd);
>        else
>                bufl = nfsnprintf(buf, sizeof(buf), "%d %s{%d%s}\r\n",
>                                  cmd->tag, cmd->cmd, cmd->cb.dlen,
>                                  CAP(LITERALPLUS) ? "+" : "");
>
> i.e., putting the easier case first and avoiding a variable format string.

Yeah, that version looks better.

>> @@ -1086,7 +1091,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
>>               int gai;
>>               char portstr[6];
>>
>> -             snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
>> +             snprintf(portstr, sizeof(portstr), "%hu", (unsigned short)srvc->port);
>
> Why not
>
>                snprintf(portstr, sizeof(portstr), "%d", srvc->port);
>
> ?

I wasn't sure whether it needed to be %hu for the purposes of the
snprintf() call. I.e. that the resulting contents of portstr might be
different on some systems.

Maybe they won't be, then we could just use %d.

Another alternative would be to change the definition of port from int
to unsigned short in the srvc struct.

> Thanks for checking the code.

Thanks for reviewing the patch.

[PATCH maint] imap-send: Fix sprintf usage

[Jonathan Nieder]

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

When composing a command for the imap server, imap-send uses a single
nfsnprintf() invocation for brevity instead of dealing separately with
the case when there is a message to be sent and the case when there
isn’t.  The unused argument in the second case, while valid, is
confusing for static analyzers and human readers.

v1.6.4-rc0~117 (imap-send: add support for IPv6, 2009-05-25)
mistakenly used %hu as the format for an int “port”, by analogy with
existing usage for the unsigned short “addr.sin_port”.  Use %d
instead.

Noticed with clang.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Ævar Arnfjörð Bjarmason wrote:

> I wasn't sure whether it needed to be %hu for the purposes of the
> snprintf() call. I.e. that the resulting contents of portstr might be
> different on some systems.
> 
> Maybe they won't be, then we could just use %d.

It’s just a nonnegative integer. :)

Here’s the updated patch.  Untested.

 imap-send.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 1a577a0..3a02e89 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -543,9 +543,13 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,
 	while (imap->literal_pending)
 		get_cmd_result(ctx, NULL);
 
-	bufl = nfsnprintf(buf, sizeof(buf), cmd->cb.data ? CAP(LITERALPLUS) ?
-			   "%d %s{%d+}\r\n" : "%d %s{%d}\r\n" : "%d %s\r\n",
-			   cmd->tag, cmd->cmd, cmd->cb.dlen);
+	if (!cmd->cb.data)
+		bufl = nfsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag, cmd->cmd);
+	else
+		bufl = nfsnprintf(buf, sizeof(buf), "%d %s{%d%s}\r\n",
+		                  cmd->tag, cmd->cmd, cmd->cb.dlen,
+		                  CAP(LITERALPLUS) ? "+" : "");
+
 	if (Verbose) {
 		if (imap->num_in_progress)
 			printf("(%d in progress) ", imap->num_in_progress);
@@ -1086,7 +1090,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 		int gai;
 		char portstr[6];
 
-		snprintf(portstr, sizeof(portstr), "%hu", srvc->port);
+		snprintf(portstr, sizeof(portstr), "%d", srvc->port);
 
 		memset(&hints, 0, sizeof(hints));
 		hints.ai_socktype = SOCK_STREAM;
-- 
1.7.2.1.544.ga752d.dirty

Re: [PATCH maint] imap-send: Fix sprintf usage

[Ævar Arnfjörð Bjarmason]

Thanks for submitting this, especially since I was too lazy to do it
myself today. It looks good.