Re: [RFC/PATCH] imap-send: Code correctness flagged by clang
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Aug 7, 2010, 22:53 UTC
- Message-ID
- <AANLkTim4CHdVLinkw1EjXB74OJ+YW-ri4GzHMNhRd+Cy@mail.gmail.com>
- In-Reply-To
- <20100807210429.GA2216@burratino>
On Sat, Aug 7, 2010 at 21:04, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 43 quoted lines
> Æ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.
Show 27 quoted lines
>> +++ 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.
Show 12 quoted lines
>> @@ -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.