Re: mdocml: To remove the const qualifier from a pointer to an object

[Ingo Schwarze <schwarze@usta.de>]

Hi Joerg,

Joerg Sonnenberger wrote on Sat, Jul 16, 2016 at 06:37:40PM +0200:
> On Sat, Jul 16, 2016 at 12:08:46AM +0200, Ingo Schwarze wrote:
>> Joerg Sonnenberger wrote:

>>> (1) It makes auditing easier by giving something simple to look for.

>> It makes auditing harder due to obfuscation.  What a plain cast
>> does is obvious on first sight.  A macro needs to be looked up when
>> auditing the code to understand what it does, and due to the
>> unnecessary quadruple cast const char **" -> const void * ->
>> intptr_t -> void * -> char * const * instead of one single cast
>> it makes auditing even harder.

> This is plainly wrong. A type cast can have a hundred different
> meanings. It is far from obvious that it is only meant to remove
> a type qualifier.

We are talking about code like this:

	const char	*ccp;
	[...]
	something involving (char *)ccp;
  
It's completely obvious not only what the intention is, but in addition
to that, the code exactly fulfills the intention:  It changes nothing
but removing the qualifier.

> If you can't derive the intention from a macro called UNCONST...

As shown above, the intention *is* obvious even without having a
macro called UNCONST.  Even worse than that, the name UNCONST is
an outrageous lie.  The macro does much more than merely removing
the const qualifier.  Consider

	const struct timeval	*tv;
	struct timespec		*ts;
	[...]
	ts = UNCONST(tv);

Such code can have disastrous consequences because on some platforms,
suseconds_t may be narrower than long, so here we have a potential
write buffer overflow.  But due to the excessively powerful UNCONST
macro, the compiler will not warn, whereas

	ts = (struct timeval *)tv;

is likely to cause a warning.  The intent of the latter is just as
obvious, if not more obvious, and it is less dangerous because it
only does what is intended to do and nothing unintended besides.

>> I see no point in specifically auditing for casts removing const.

> Given that const memory can actually be read-only, removing the logical
> property is a clear sign of either a misdesigned interface, a workaround
> for a language defect or a plain bug to be waiting.

These bugs are usually relatively benign, though.  You write to
immutable memory?  SIGSEGF.  Easy to find, easy to fix, rarely
exploitable.  Besides, such bugs are rare.  I have seen lots and
lots of buffer overruns and signedness issues, many of them very
dangerous, and very few bogus const castaways, if any.

> It is no less a warning sign than use of strcpy..

Bad comparison:  That usually leads to write buffer overruns and
no segfault - hard to find, often exploitable.

Many type conversion issues are actually notorious for being
security critical:  Signedness, width mismatch, overflows...
Bogus loss of const, by constrast, is relatively benign, so
UNCONST increases the risk of unintentionally hiding width and
signedness issues, just to make something rather harmless stand
out prominently.

> It is used in production builds by many systems, including FreeBSD and
> NetBSD, for a lot of software. Just because you don't do that in
> OpenBSD doesn't mean it doesn't create problems for others.

The problem is easy to solve, though, by simply adding -Wno-cast-qual
to the mandoc Makefile, without disturbing anything in the rest of
your tree.

I'm not sure how useful it is to continue the discussion.
In a nutshell, we disagree regarding a style issue.
There are arguments either way; the ones against UNCONST seem
stronger to me, and so far, you claim the opposite.

Of course, it would be nice if we could find an agreement, but it's
not the end of the world if we don't...

Yours,
  Ingo
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv