References: <RT-Ticket-46132@isc.org> <rt-4.4.1-33589-1506644880-237.46132-4-0@isc.org> <rt-4.4.1-30085-1506647021-124.46132-4-0@isc.org>
content-type: text/plain; charset="utf-8"
Subject: Re: [ISC-Bugs #46132] remove more sprintf's
In-Reply-To: <rt-4.4.1-30085-1506647021-124.46132-4-0@isc.org>
From: "Mark Andrews" <marka@isc.org>
Date: Fri, 29 Sep 2017 11:31:20 +1000
MIME-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
To: bind9-public@isc.org
From marka@isc.org  Fri Sep 29 01:32:45 2017
X-Original-To: bind9-public@bugs.isc.org
X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on mx.ams1.isc.org
Content-Transfer-Encoding: quoted-printable
X-RT-Original-Encoding: utf-8
Message-ID: <8D5DC866-8BE9-440A-8496-43240AE58304@isc.org>
X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00, RP_MATCHES_RCVD autolearn=ham autolearn_force=no version=3.4.1
RT-Message-ID: <rt-4.4.1-30085-1506648778-1890.46132-0-0@isc.org>
Content-Length: 3082


> On 29 Sep 2017, at 11:03 am, Ondrej Sury via RT <bind9-public@isc.org> wrote:
> 
> Thank you for doing this, it mostly look good.
> 
> Also forgive me if my comments doesn't make sense, as I don't have a deep understanding of BIND 9 source code, so you'll have to be patient with me.
> 
> A general comment - you never change the snprintf return value, so the assumption is that the resulting string always fits from the previous usage of sprintf (otherwise it would smash the stack), but is the assumption always true? Especially in the places where the correct output matters (like totext_* functions).

The code was already correct.  The problem is that OS developers have made
the linker so noisy it is hard to see real issues for false positives.

> One little nitpick - in lib/dns/gssapictx.c
> 
> Could you change this:
> 
> +                       size = strlen(gssapi_keytab) + 13;
> +                       kt = malloc(size);
> 
> to:
> 
> +                       size_t size = strlen(gssapi_keytab) + sizeof("KRB5_KTNAME=") + 1;
> +                       char *kt = malloc(size);

I dislike initialisations with declarations that depend on each other.  Additionally
the second is not safe from “sort(declaration block)”.

> 
> --
> 
> I am curious about changes in lib/dns/rdata/generic/loc_29.c
> 
> While I am generally in favour of using curly braces even around single statements (as I do believe it makes the code more readable and a tad bit safer for mistakes), this is not really consistent with the rest of the coding style found in the BIND 9.
> 
> --
> 
> Again a nitpick (if we are going to go toward C11), in lib/isc/inet_ntop.c, could you please create the variable close to the usage, e.g. change:
> 
> +       int n;
> +
> +       n = snprintf(tmp, sizeof(tmp), fmt, src[0], src[1], src[2], src[3]);
> 
> to single line:
> 
> +       int n = snprintf(tmp, sizeof(tmp), fmt, src[0], src[1], src[2], src[3])

We still only require C99 and even there declarations need to be at
the start of a block as the Windows compilers blow up.

> --
> 
> dtto in lib/isc/log.c and why is size unsigned int and not size_t?

changed.

> --
> 
> In lib/isccfg/parser.c, it seems to me that this change could be rewritten as:
> 
> -               message[sizeof(message) - sizeof(ELIPSIS)] = 0;
> -               strlcat(message, ELIPSIS, sizeof(message));
> +               strncpy(message + sizeof(message) - sizeof(ELIPSIS), ELIPSIS, sizeof(ELIPSIS));
> 
> strlcat() can be used, but the result should be same.  It would save one iteration through 2048 sized buffer every time the log message overflows

strncpy() also flags with compiler/linker warnings as it is much more often
misused than even strcpy.

The original strlcat usage meets the spirit of the reasons for strlcat
by clearly ensuring that message isn’t overflowed and we rarely overflow.

> -- 
> Ticket History: https://bugs.isc.org/Ticket/Display.html?id=46132

-- 
Mark Andrews, ISC
1 Seymour St., Dundas Valley, NSW 2117, Australia
PHONE: +61 2 9871 4742              INTERNET: marka@isc.org