{"thread":{"id":"24661","subject":"[RFC/PATCH] imap-send: Code correctness flagged by clang","startedAt":"2010-08-07T12:12:16Z","lastAt":"2010-08-07T23:25:33Z","messageCount":5,"participants":["Ævar Arnfjörð Bjarmason","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"147385","messageId":"1281183136-10352-1-git-send-email-avarab@gmail.com","threadId":"24661","inReplyTo":null,"subject":"[RFC/PATCH] imap-send: Code correctness flagged by clang","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-08-07T12:12:16Z","receivedAt":"2010-08-07T12:12:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Clang 1.1 flagged the following issues in imap-send.c, this change\nfixes the warnings by moving some code around:\n\n    imap-send.c:548:27: warning: data argument not used by format string [-Wformat-extra-args]\n                               cmd->tag, cmd->cmd, cmd->cb.dlen);\n                                                   ^\n\nHere the sprintf format didn't use the cmd->cb.dlen argument if\ncmd->cb.data was false. Change the code to use a if/else instead of a\ntwo-level ternary to work it. This code was introduced with imap-send\nitself in f2561fda.\n\n    imap-send.c:1089:41: warning: conversion specifies type 'unsigned short' but the argument has type 'int' [-Wformat]\n                    snprintf(portstr, sizeof(portstr), \"%hu\", srvc->port);\n                                                        ~~^   ~~~~~~~~~~\n\nHere sprintf is being given an int with a %hu format. Cast the\nsrvc->port to unsigned short to work it. This code was introduced in\n94ad2437 to add IPv6 support.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n imap-send.c |   13 +++++++++----\n 1 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 1a577a0..4b25375 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -543,9 +543,14 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,\n \twhile (imap->literal_pending)\n \t\tget_cmd_result(ctx, NULL);\n \n-\tbufl = nfsnprintf(buf, sizeof(buf), cmd->cb.data ? CAP(LITERALPLUS) ?\n-\t\t\t   \"%d %s{%d+}\\r\\n\" : \"%d %s{%d}\\r\\n\" : \"%d %s\\r\\n\",\n-\t\t\t   cmd->tag, cmd->cmd, cmd->cb.dlen);\n+\tif (cmd->cb.data) {\n+\t\tbufl = nfsnprintf(buf, sizeof(buf),\n+\t\t                  CAP(LITERALPLUS) ? \"%d %s{%d+}\\r\\n\" : \"%d %s{%d}\\r\\n\",\n+\t\t                  cmd->tag, cmd->cmd, cmd->cb.dlen);\n+\t} else {\n+\t\tbufl = nfsnprintf(buf, sizeof(buf), \"%d %s\\r\\n\", cmd->tag, cmd->cmd);\n+\t}\n+\n \tif (Verbose) {\n \t\tif (imap->num_in_progress)\n \t\t\tprintf(\"(%d in progress) \", imap->num_in_progress);\n@@ -1086,7 +1091,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)\n \t\tint gai;\n \t\tchar portstr[6];\n \n-\t\tsnprintf(portstr, sizeof(portstr), \"%hu\", srvc->port);\n+\t\tsnprintf(portstr, sizeof(portstr), \"%hu\", (unsigned short)srvc->port);\n \n \t\tmemset(&hints, 0, sizeof(hints));\n \t\thints.ai_socktype = SOCK_STREAM;\n-- \n1.7.1\n"},{"id":"147397","messageId":"20100807210429.GA2216@burratino","threadId":"24661","inReplyTo":"1281183136-10352-1-git-send-email-avarab@gmail.com","subject":"Re: [RFC/PATCH] imap-send: Code correctness flagged by clang","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-07T21:04:29Z","receivedAt":"2010-08-07T21:04:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n\n> [Subject: imap-send: Code correctness flagged by clang]\n>\n> Clang 1.1 flagged the following issues in imap-send.c, this change\n> fixes the warnings by moving some code around:\n> \n>     imap-send.c:548:27: warning: data argument not used by format string [-Wformat-extra-args]\n>                                cmd->tag, cmd->cmd, cmd->cb.dlen);\n>                                                    ^\n> \n> Here the sprintf format didn't use the cmd->cb.dlen argument if\n> cmd->cb.data was false. Change the code to use a if/else instead of a\n> two-level ternary to work it. This code was introduced with imap-send\n> itself in f2561fda.\n> \n>     imap-send.c:1089:41: warning: conversion specifies type 'unsigned short' but the argument has type 'int' [-Wformat]\n>                     snprintf(portstr, sizeof(portstr), \"%hu\", srvc->port);\n>                                                         ~~^   ~~~~~~~~~~\n> \n> Here sprintf is being given an int with a %hu format. Cast the\n> srvc->port to unsigned short to work it. This code was introduced in\n> 94ad2437 to add IPv6 support.\n\nNitpick: that this was found by clang is probably not the first thing\na person trying to figure out what the patch does needs to know.\nMaybe:\n\n\tSubject: imap-send: Fix sprintf usage\n\n\tWhen composing a command for the imap server, imap-send\n\tuses a single nfsnprintf() invocation for brevity\n\tinstead of dealing separately with the case when there\n\tis a message to be sent and the case when there isn’t.\n\tThe unused argument in the second case, while valid,\n\tis confusing for static analyzers and human readers.\n\n\tv1.6.4-rc0~117 (imap-send: add support for IPv6, 2009-05-25)\n\tmistakenly used %hu as the format for an int “port”, by\n\tanalogy with existing usage for the unsigned short\n\t“addr.sin_port”.  Use %d instead.\n\n\tNoticed with clang.\n\n> +++ b/imap-send.c\n> @@ -543,9 +543,14 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,\n>  \twhile (imap->literal_pending)\n>  \t\tget_cmd_result(ctx, NULL);\n>  \n> -\tbufl = nfsnprintf(buf, sizeof(buf), cmd->cb.data ? CAP(LITERALPLUS) ?\n> -\t\t\t   \"%d %s{%d+}\\r\\n\" : \"%d %s{%d}\\r\\n\" : \"%d %s\\r\\n\",\n> -\t\t\t   cmd->tag, cmd->cmd, cmd->cb.dlen);\n> +\tif (cmd->cb.data) {\n> +\t\tbufl = nfsnprintf(buf, sizeof(buf),\n> +\t\t                  CAP(LITERALPLUS) ? \"%d %s{%d+}\\r\\n\" : \"%d %s{%d}\\r\\n\",\n> +\t\t                  cmd->tag, cmd->cmd, cmd->cb.dlen);\n> +\t} else {\n> +\t\tbufl = nfsnprintf(buf, sizeof(buf), \"%d %s\\r\\n\", cmd->tag, cmd->cmd);\n> +\t}\n> +\n\nHmm, maybe this would be easier to read:\n\n\tif (!cmd->cb.data)\n\t\tbufl = nfsnprintf(buf, sizeof(buf), \"%d %s\\r\\n\", cmd->tag, cmd->cmd);\n\telse\n\t\tbufl = nfsnprintf(buf, sizeof(buf), \"%d %s{%d%s}\\r\\n\",\n\t\t                  cmd->tag, cmd->cmd, cmd->cb.dlen,\n\t\t                  CAP(LITERALPLUS) ? \"+\" : \"\");\n\ni.e., putting the easier case first and avoiding a variable format string.\n\n> @@ -1086,7 +1091,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)\n>  \t\tint gai;\n>  \t\tchar portstr[6];\n>  \n> -\t\tsnprintf(portstr, sizeof(portstr), \"%hu\", srvc->port);\n> +\t\tsnprintf(portstr, sizeof(portstr), \"%hu\", (unsigned short)srvc->port);\n\nWhy not\n\n\t\tsnprintf(portstr, sizeof(portstr), \"%d\", srvc->port);\n\n?\n\nThanks for checking the code.\n"},{"id":"147401","messageId":"AANLkTim4CHdVLinkw1EjXB74OJ+YW-ri4GzHMNhRd+Cy@mail.gmail.com","threadId":"24661","inReplyTo":"20100807210429.GA2216@burratino","subject":"Re: [RFC/PATCH] imap-send: Code correctness flagged by clang","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-08-07T22:53:51Z","receivedAt":"2010-08-07T22:53:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Aug 7, 2010 at 21:04, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Ævar Arnfjörð Bjarmason wrote:\n>\n>> [Subject: imap-send: Code correctness flagged by clang]\n>>\n>> Clang 1.1 flagged the following issues in imap-send.c, this change\n>> fixes the warnings by moving some code around:\n>>\n>>     imap-send.c:548:27: warning: data argument not used by format string [-Wformat-extra-args]\n>>                                cmd->tag, cmd->cmd, cmd->cb.dlen);\n>>                                                    ^\n>>\n>> Here the sprintf format didn't use the cmd->cb.dlen argument if\n>> cmd->cb.data was false. Change the code to use a if/else instead of a\n>> two-level ternary to work it. This code was introduced with imap-send\n>> itself in f2561fda.\n>>\n>>     imap-send.c:1089:41: warning: conversion specifies type 'unsigned short' but the argument has type 'int' [-Wformat]\n>>                     snprintf(portstr, sizeof(portstr), \"%hu\", srvc->port);\n>>                                                         ~~^   ~~~~~~~~~~\n>>\n>> Here sprintf is being given an int with a %hu format. Cast the\n>> srvc->port to unsigned short to work it. This code was introduced in\n>> 94ad2437 to add IPv6 support.\n>\n> Nitpick: that this was found by clang is probably not the first thing\n> a person trying to figure out what the patch does needs to know.\n> Maybe:\n>\n>        Subject: imap-send: Fix sprintf usage\n>\n>        When composing a command for the imap server, imap-send\n>        uses a single nfsnprintf() invocation for brevity\n>        instead of dealing separately with the case when there\n>        is a message to be sent and the case when there isn’t.\n>        The unused argument in the second case, while valid,\n>        is confusing for static analyzers and human readers.\n>\n>        v1.6.4-rc0~117 (imap-send: add support for IPv6, 2009-05-25)\n>        mistakenly used %hu as the format for an int “port”, by\n>        analogy with existing usage for the unsigned short\n>        “addr.sin_port”.  Use %d instead.\n>\n>        Noticed with clang.\n\nThat looks better.\n\n>> +++ b/imap-send.c\n>> @@ -543,9 +543,14 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,\n>>       while (imap->literal_pending)\n>>               get_cmd_result(ctx, NULL);\n>>\n>> -     bufl = nfsnprintf(buf, sizeof(buf), cmd->cb.data ? CAP(LITERALPLUS) ?\n>> -                        \"%d %s{%d+}\\r\\n\" : \"%d %s{%d}\\r\\n\" : \"%d %s\\r\\n\",\n>> -                        cmd->tag, cmd->cmd, cmd->cb.dlen);\n>> +     if (cmd->cb.data) {\n>> +             bufl = nfsnprintf(buf, sizeof(buf),\n>> +                               CAP(LITERALPLUS) ? \"%d %s{%d+}\\r\\n\" : \"%d %s{%d}\\r\\n\",\n>> +                               cmd->tag, cmd->cmd, cmd->cb.dlen);\n>> +     } else {\n>> +             bufl = nfsnprintf(buf, sizeof(buf), \"%d %s\\r\\n\", cmd->tag, cmd->cmd);\n>> +     }\n>> +\n>\n> Hmm, maybe this would be easier to read:\n>\n>        if (!cmd->cb.data)\n>                bufl = nfsnprintf(buf, sizeof(buf), \"%d %s\\r\\n\", cmd->tag, cmd->cmd);\n>        else\n>                bufl = nfsnprintf(buf, sizeof(buf), \"%d %s{%d%s}\\r\\n\",\n>                                  cmd->tag, cmd->cmd, cmd->cb.dlen,\n>                                  CAP(LITERALPLUS) ? \"+\" : \"\");\n>\n> i.e., putting the easier case first and avoiding a variable format string.\n\nYeah, that version looks better.\n\n>> @@ -1086,7 +1091,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)\n>>               int gai;\n>>               char portstr[6];\n>>\n>> -             snprintf(portstr, sizeof(portstr), \"%hu\", srvc->port);\n>> +             snprintf(portstr, sizeof(portstr), \"%hu\", (unsigned short)srvc->port);\n>\n> Why not\n>\n>                snprintf(portstr, sizeof(portstr), \"%d\", srvc->port);\n>\n> ?\n\nI wasn't sure whether it needed to be %hu for the purposes of the\nsnprintf() call. I.e. that the resulting contents of portstr might be\ndifferent on some systems.\n\nMaybe they won't be, then we could just use %d.\n\nAnother alternative would be to change the definition of port from int\nto unsigned short in the srvc struct.\n\n> Thanks for checking the code.\n\nThanks for reviewing the patch.\n"},{"id":"147404","messageId":"20100807230945.GA3427@burratino","threadId":"24661","inReplyTo":"AANLkTim4CHdVLinkw1EjXB74OJ+YW-ri4GzHMNhRd+Cy@mail.gmail.com","subject":"[PATCH maint] imap-send: Fix sprintf usage","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-07T23:09:45Z","receivedAt":"2010-08-07T23:09:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n\nWhen composing a command for the imap server, imap-send uses a single\nnfsnprintf() invocation for brevity instead of dealing separately with\nthe case when there is a message to be sent and the case when there\nisn’t.  The unused argument in the second case, while valid, is\nconfusing for static analyzers and human readers.\n\nv1.6.4-rc0~117 (imap-send: add support for IPv6, 2009-05-25)\nmistakenly used %hu as the format for an int “port”, by analogy with\nexisting usage for the unsigned short “addr.sin_port”.  Use %d\ninstead.\n\nNoticed with clang.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nÆvar Arnfjörð Bjarmason wrote:\n\n> I wasn't sure whether it needed to be %hu for the purposes of the\n> snprintf() call. I.e. that the resulting contents of portstr might be\n> different on some systems.\n> \n> Maybe they won't be, then we could just use %d.\n\nIt’s just a nonnegative integer. :)\n\nHere’s the updated patch.  Untested.\n\n imap-send.c |   12 ++++++++----\n 1 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 1a577a0..3a02e89 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -543,9 +543,13 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,\n \twhile (imap->literal_pending)\n \t\tget_cmd_result(ctx, NULL);\n \n-\tbufl = nfsnprintf(buf, sizeof(buf), cmd->cb.data ? CAP(LITERALPLUS) ?\n-\t\t\t   \"%d %s{%d+}\\r\\n\" : \"%d %s{%d}\\r\\n\" : \"%d %s\\r\\n\",\n-\t\t\t   cmd->tag, cmd->cmd, cmd->cb.dlen);\n+\tif (!cmd->cb.data)\n+\t\tbufl = nfsnprintf(buf, sizeof(buf), \"%d %s\\r\\n\", cmd->tag, cmd->cmd);\n+\telse\n+\t\tbufl = nfsnprintf(buf, sizeof(buf), \"%d %s{%d%s}\\r\\n\",\n+\t\t                  cmd->tag, cmd->cmd, cmd->cb.dlen,\n+\t\t                  CAP(LITERALPLUS) ? \"+\" : \"\");\n+\n \tif (Verbose) {\n \t\tif (imap->num_in_progress)\n \t\t\tprintf(\"(%d in progress) \", imap->num_in_progress);\n@@ -1086,7 +1090,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)\n \t\tint gai;\n \t\tchar portstr[6];\n \n-\t\tsnprintf(portstr, sizeof(portstr), \"%hu\", srvc->port);\n+\t\tsnprintf(portstr, sizeof(portstr), \"%d\", srvc->port);\n \n \t\tmemset(&hints, 0, sizeof(hints));\n \t\thints.ai_socktype = SOCK_STREAM;\n-- \n1.7.2.1.544.ga752d.dirty\n"},{"id":"147405","messageId":"AANLkTinD=cqqzG+d8UxoJo1TLhmuu-3L5+3y0WUT5xas@mail.gmail.com","threadId":"24661","inReplyTo":"20100807230945.GA3427@burratino","subject":"Re: [PATCH maint] imap-send: Fix sprintf usage","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-08-07T23:25:33Z","receivedAt":"2010-08-07T23:25:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Thanks for submitting this, especially since I was too lazy to do it\nmyself today. It looks good.\n"}]}