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
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
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