[dns-operations] Robert Graham - A Quick Review of the BIND9 Code.

[Robert Edmonds]

The gcc optimization is opportunistic.  It can only affect the object
code that is generated by the compiler.  Incidentally, constant
propagation doesn't really have much to do with the 'const' qualifier:

    https://en.wikipedia.org/wiki/Constant_folding#Constant_propagation

The 'const' qualifier has to be consciously added to a function
prototype by the programmer when he or she designs the API.  Paul is
asking if there are any tools which can flag candidate functions where
it makes sense to retroactively add the 'const' qualifier.  Compiler
passes that silently optimize the object code are not that tool.

The big advantage of using the 'const' qualifier is that C compilers can
flag violations of const-correctness at *compile* time, with
diagnostics; that makes it easy for those bugs to be detected and fixed
during the development process.  The assertions in the BIND code are
executed at *run* time; bugs that are only detected by those assertions
can only be detected by running the compiled executable.

Rob Graham's article mentions the 'const' thing only in passing, though:

    A quick grep shows that lack of *const correctness* is pretty common
    throughout the BIND9 source code. Every quality guide in the world
    strongly suggests *const correctness* -- that's it's lacking here
    hints at larger problems.

Jim Popovitch wrote:
> Forgive me if I am misunderstanding your statements, but API's don't
> interface with the source code, they interface with the object code
> that is produced by a compiler.  GCC's -O2 optimizations produce
> compiled code that has corrected/added const declarations.   So, even
> if the published API docs say the interface func is non-const, the
> underlying API object code (if produced by gcc -O2) will treat the
> func as being const (which will probably break a poorly documented API
> call).   To be clear, I mentioned -fipa-cp and -O2 because Paul was
> looking for a way to detect undefined consts.   Gcc -O2 also has a
> plethora of other optimizations that pertain to const
> optimization/correction, -fipa-cp-clone (-03) would probably address
> the issue with API calls.
> 
> Back to the article, (which btw doesn't mention API calls), the const
> issue was with dns_message_findname(), which gcc -O2 would most likely
> auto-declare as const.
> 
> A cursory look of Debian's Bind9 build environment shows:
>    $ grep CFLAGS Makefile | grep O2
>    CFLAGS =        -g -O2 -I/usr/include/libxml2
>    BUILD_CFLAGS = -g -O2 -I/usr/include/libxml2
> 
> I don't know if ISC publishes recommended compile options, I would
> think that would be distro/release specific.
> 
> Finally, a quick look at Bind9 v9.8.4 source shows a considerable
> amount of type checking/validation occurring i.e.
> REQUIRE(VALID_FCTX()), REQUIRE(VALID_IFITER()),
> REQUIRE(VALID_SOCKET()), etc.   So just the single lack of a const
> declaration in no part means that there is an unchecked variable.
> Even this "vulnerability" shows that nothing was at stake other than a
> properly working assert/REQUIRE().   Ok, so the assert caused bind9 to
> crash... but it's not like it leaked keys.
> 
> -Jim P.

-- 
Robert Edmonds