Bug #46132 for bind9-public: remove more sprintf's

Thu, 28 Sep 2017 20:28:00 -0400 Mark Andrews <marka@isc.org> - Ticket created

Subject:	remove more sprintf's
From:	marka@isc.org
To:	bind9-public@isc.org
Date:	Thu, 28 Sep 2017 14:28:00 -1000

Thu, 28 Sep 2017 20:35:17 -0400 Mark Andrews <marka@isc.org> - Status changed from 'open' to 'review'

Thu, 28 Sep 2017 21:03:41 -0400 Ondrej Sury <ondrej@isc.org> - Correspondence added

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). 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 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]); -- dtto in lib/isc/log.c and why is size unsigned int and not size_t? -- 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.

Thu, 28 Sep 2017 21:32:59 -0400 Mark Andrews <marka@isc.org> - Correspondence added

Subject:	Re: [ISC-Bugs #46132] remove more sprintf's
From:	"Mark Andrews" <marka@isc.org>
Date:	Fri, 29 Sep 2017 11:31:20 +1000
To:	bind9-public@isc.org

> 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

Fri, 29 Sep 2017 01:33:38 -0400 Ondrej Sury <ondrej@isc.org> - Correspondence added

To:	bind9-public@isc.org
Subject:	Re: [ISC-Bugs #46132] remove more sprintf's
Date:	Thu, 28 Sep 2017 22:31:39 -0700
From:	"Ondřej Surý" <ondrej@isc.org>

On 28 Sep 2017, at 18:33, Mark Andrews via RT <bind9-public@isc.org> wrote:

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.

I do believe they are right. Although the code was correct, it’s easy to make a mistake when there’s more people in the team and it’s just better to play to a safe side and don’t ever use sprintf as a reasonable precaution to not let anybody else fall into the trap.

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)”.

On the other hand, keeping the declaration close the place where it’s used makes the code more readable. But fair enough, let’s not overengineer this. I would prefer though at least using the sizeof("KRB5_KTNAME=“) part, as this would make it consistent with the rest of your code.

As a side note - do you ever do “sort(declaration block)”? I’ve seen people shuffle the declaration block to be logically arranged, but never to “sort it”.

--

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.

Fair enough.

--

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.

Sorry, what I really wanted to write was

memcpy(message + sizeof(message) - sizeof(ELIPSIS), ELIPSIS, sizeof(ELIPSIS));

This would be always correct. But again this is no big issue, just a nitpick. I’ll leave it to your discretion.

Otherwise go ahead and merge.

O.

signature.asc

Message body not shown because it is not plain text.

Tue, 03 Oct 2017 00:02:07 -0400 Mark Andrews <marka@isc.org> - Version Fixed 9.9.12, 9.9.12(sub), 9.10.7, 9.10.7(sub), 9.11.3, 9.12.0 added

Tue, 03 Oct 2017 00:02:07 -0400 Mark Andrews <marka@isc.org> - Correspondence added

4748. [cleanup] Sprintf to snprintf coversions. [RT #46132]

Tue, 03 Oct 2017 00:02:08 -0400 Mark Andrews <marka@isc.org> - Status changed from 'review' to 'resolved'

Created:	Thu, 28 Sep 2017 20:28:00 -0400
Updated:	Tue, 03 Oct 2017 00:02:08 -0400
Closed:	Tue, 03 Oct 2017 00:02:08 -0400

Bug #46132 for bind9-public: remove more sprintf's

This bug tracker is no longer active.