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

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

[Ingo Schwarze]

Hi Joerg,

we seem to disagree here.

Joerg Sonnenberger wrote on Fri, Jul 15, 2016 at 11:31:03PM +0200:
> On Fri, Jul 15, 2016 at 02:33:31PM -0500, schwarze@mdocml.bsd.lv wrote:

>> To remove the const qualifier from a pointer to an object - either 
>> because we know it is actually mutable or because we are passing
>> it to a function that doesn't accept a const object but won't
>> actually attempt to modify it - simply casting from (const type *)
>> to (type *) is legal C and clearly expresses the intent.  
>> So get rid of the obfuscating UNCONST macro.
>> Basic idea discussed with guenther@.

> This is completely wrong and a clear regression.

There is no regression.  The code works fine after the change.

> The macro exists

The macro does not exist.  Neither the C standard nor POSIX specify
it or even indicate that something like that might be needed or
even useful.  It was a purely local quirk in one single place of
the mandoc codebase.  Besides, i checked with Philip Guenther, who
is both an expert in C and in POSIX, that no contortions are needed
here.

> for two reasons:
> (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.

I see no point in specifically auditing for casts removing const.
Sure, auditing for type conversion issues is useful, but many type
conversions, particularly implicit ones, are much more dangerous
than const removals.  Auditing for type conversion errors is very
hard (think of signedness and width issues).  Introducing an ugly
macro to make one of the lesser problems stand out prominently
doesn't help at all.

> (2) The macro actually stopped -Wcast warnings.

You mean, -Wcast-qual?  That's not enabled by default for a reason:
It's not useful in production builds.  (It may occasionally be
helpful when doing a one-time audit of a dubious code base to
identify candidates for erroneous casts, though.)

> The replacement does not.

Just don't enable -Wcast-qual for compiling mandoc.  I consider
that option harmful because it drives developers toward obfuscation.

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

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

[Joerg Sonnenberger]

On Sat, Jul 16, 2016 at 12:08:46AM +0200, Ingo Schwarze wrote:
> > The macro exists
> 
> The macro does not exist.  Neither the C standard nor POSIX specify
> it or even indicate that something like that might be needed or
> even useful.  It was a purely local quirk in one single place of
> the mandoc codebase.  Besides, i checked with Philip Guenther, who
> is both an expert in C and in POSIX, that no contortions are needed
> here.

This doesn't make any sense at all given that you just removed the
macro.

> > for two reasons:
> > (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. If you can't derive the intention from a macro called
UNCONST...

> 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. It is no less a
warning sign than use of strcpy..

> > (2) The macro actually stopped -Wcast warnings.
> 
> You mean, -Wcast-qual?  That's not enabled by default for a reason:
> It's not useful in production builds.  (It may occasionally be
> helpful when doing a one-time audit of a dubious code base to
> identify candidates for erroneous casts, though.)

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 replacement does not.
> 
> Just don't enable -Wcast-qual for compiling mandoc.  I consider
> that option harmful because it drives developers toward obfuscation.

I beg to differ and a lot of people agree with me.

Joerg
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

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

[Ingo Schwarze]

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