[RFA] [PATCH] Add a warning for invalid function casts

[Martin Sebor]

On 10/05/2017 03:04 PM, Bernd Edlinger wrote:
> On 10/05/17 18:16, Martin Sebor wrote:
>> On 10/03/2017 01:33 PM, Bernd Edlinger wrote:
>>>
>>> I'm not sure if this warning may be a bit too strict, but I think
>>> so far it just triggered on rather questionable code.
>>>
>>> Thoughts?
>>
>> My initial thought is that although casts between incompatible
>> function types undoubtedly mask bugs, the purpose of such casts
>> is to make such conversions possible.  Indiscriminately diagnosing
>> them would essentially eliminate this feature of the type system.
>> Having to add another cast to a different type(*) to suppress
>> the warning when such a conversion is intended doesn't seem like
>> a good solution (the next logical step to find bugs in those
>> conversions would then be to add another warning to see through
>> those additional casts, and so on).
>>
>> With that, my question is: under what circumstances does the
>> warning not trigger on a cast to a function of an incompatible
>> type?
>>
>> In my (very quick) tests the warning appears to trigger on all
>> strictly incompatible conversions, even if they are otherwise
>> benign, such as:
>>
>>    int f (const int*);
>>    typedef int F (int*);
>>
>>    F* pf1 = f;        // -Wincompatible-pointer-types
>>    F* pf2 = (F*)f;    // -Wcast-function-type
>>
>> Likewise by:
>>
>>    int f (signed char);
>>    typedef int F (unsigned char);
>>
>>    F* pf = (F*)f;    // -Wcast-function-type
>>
>> I'd expect these conversions to be useful and would view warning
>> for them with -Wextra as excessive.  In fact, I'm not sure I see
>> the benefit of warning on these casts under any circumstances.
>>
>> Similarly, for casts between pointers to the same integer type
>> with a different sign, or those involving ILP32/LP64 portability
>> issues I'd expect the warning not to trigger unless requested
>> (e.g., by some other option targeting those issues).
>>
>> So based on these initial observations and despite the bugs it
>> may have uncovered, I share your concern that the warning in
>> its present form is too strict to be suitable for -Wextra, or
>> possibly even too noisy to be of practical use on its own and
>> outside of -Wextra.
>>
>> Out of curiosity, have you done any tests on other code bases
>> besides GCC to see how many issues (true and false positives)
>> it finds?
>>
>> Martin
>>
>> [*] Strictly speaking, the semantics of casting a function
>> pointer to intptr_t aren't necessarily well-defined.  Only void*
>> can be portably converted to intptr_t and back, and only object
>> pointers are required to be convertible to void* and back.  And
>> although GCC defines the semantics of these conversions, forcing
>> programs to abandon a well defined language feature in favor of
>> one that's not as cleanly specified would undermine the goal
>> the warning is meant to achieve.
>
> Thanks for your advice.
>
> I did look into openssl and linux, both have plenty of
> -Wcast-function-type warnings.
>
> In the case of openssl it is lots of similar stuff
> crypto/aes/aes_cfb.c:25:27: warning: cast between incompatible function
> types from 'void (*)(const unsigned char *, unsigned char *, const
> AES_KEY *) {aka void (*)(const unsigned char *, unsigned char *, const
> struct aes_key_st *)}' to 'void (*)(const unsigned char *, unsigned
> char *, const void *)' [-Wcast-function-type]
>                             (block128_f) AES_encrypt);
>
> The problem is the function is of course called with a different
> signature than what is declared.  They take it somehow for granted,
> that "void*" or "const void*" parameter are an alias for
> any pointer or any const pointer.  Either as parameter or as return
> code.
>
> I would believe this is not well-defined by the c-standard.
>
> But it makes the warning less useful because it would be impossible
> to spot the few places where the call will actually abort at
> runtime.
>
> Then I tried to compile linux, I noticed that there is a new
> warning for the alias to incompatible function.
>
> I saw it very often, and it is always when a system call
> is defined:
>
> ./include/linux/compat.h:50:18: Warnung: »compat_sys_ptrace« alias
> between functions of incompatible types »long int(compat_long_t,
> compat_long_t,  compat_long_t,  compat_long_t) {alias long int(int,
> int,  int,  int)}« and »long int(long int,  long int,  long int,  long
> int)« [-Wattributes]
>    asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))\
>
> ./include/linux/syscalls.h:211:18: Warnung: »sys_rt_sigprocmask« alias
> between functions of incompatible types »long int(int,  sigset_t *,
> sigset_t *, size_t) {alias long int(int,  struct <anonym> *, struct
> <anonym> *, long unsigned int)}« and »long int(long int,  long int,
> long int,  long int)« [-Wattributes]
>    asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
>
>
> Needless to say that even more Wcast-function-type warning
> happen.
>
> ./include/linux/timer.h:178:23: Warnung: cast between incompatible
> function types from »void (*)(struct timer_list *)« to »void (*)(long
> unsigned int)« [-Wcast-function-type]
>    __setup_timer(timer, (TIMER_FUNC_TYPE)callback,
>
> So they assume obviously that any int / long / pointer value
> are compatible with uintptr_t and intptr_t.
>
> If the Wattribute stays this way, I can garantee that Linus will simply
> add -Wno-attribute to the linux makefiles, which is bad because
> -Wattribute is more than one warning alone.
>
> At the least we should rename this particular warning into
> something else, so that it is not necessary to disable everything
> when only this specific warning is too hard to fix.
>
> Maybe it would be good to not warn in type-casts, when they can be
> assumed to be safe, for instance
> void* <-> any pointer (parameter or result),
> uintptr_t <-> any int, any pointer (parameter or result),
> void (*) (void) and void (*) (...) <-> any function pointer.
>
> I think that applies to both warnings.
>
> What do you think?

The attribute alias/ifunc incompatibilities are conflicts between
declarations of the same symbol.  Such conflicts are invalid in C
and ordinarily rejected with a hard error.  With the exception of
ifunc resolvers returning void*, I see the alias/ifunc conflicts
as signs of bugs or, at best, benign mistakes.  I also can think
of no reason why they shouldn't in most cases be corrected to
conform to the C requirements.  Where they can't be fixed it's
usually easy to suppress the warnings by declaring the functions
without a prototype.  That also makes it clear to the reader that
type checking has been suppressed.

On the other hand, explicit casts between incompatible pointers
are a well-defined feature of the language and their use a clear
indication that the programmer was aware of the incompatibility
and intentionally suppressed it.  There no doubt are bugs among
these that would be helpful to find so I think the challenge
here is to distinguish the intentional and benign conversions
from the others while minimizing false positives.  Since there
are likely to be a large proportion of the intentional and benign
cases, unless the heuristic to find the others is very good, there
should also be an easy way to suppress the false positives without
compromising readability.

Martin

PS We are discussing the kernel warnings in bug 82435.  I've
already moved them under -Wincompatible-pointer-types but it
sounds like a new option altogether might be preferable.
Ideally, though, if it's possible it would be best to fix them
like we did for Glibc so the kernel too would benefit from the
better type checking.  I should look into that.