Re: [PATCH] Add a warning for invalid function casts

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

Ping...

for the latest version of my patch which can be found here:

https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00559.html


Thanks
Bernd.

On 10/10/17 00:30, Bernd Edlinger wrote:
> On 10/09/17 20:34, Martin Sebor wrote:
>> On 10/09/2017 11:50 AM, Bernd Edlinger wrote:
>>> On 10/09/17 18:44, Martin Sebor wrote:
>>>> On 10/07/2017 10:48 AM, Bernd Edlinger wrote:
>>>>> Hi!
>>>>>
>>>>> I think I have now something useful, it has a few more heuristics
>>>>> added, to reduce the number of false-positives so that it
>>>>> is able to find real bugs, for instance in openssl it triggers
>>>>> at a function cast which has already a TODO on it.
>>>>>
>>>>> The heuristics are:
>>>>> - handle void (*)(void) as a wild-card function type.
>>>>> - ignore volatile, const qualifiers on parameters/return.
>>>>> - handle any pointers as equivalent.
>>>>> - handle integral types, enums, and booleans of same precision
>>>>>     and signedness as equivalent.
>>>>> - stop parameter validation at the first "...".
>>>>
>>>> These sound quite reasonable to me.  I have a reservation about
>>>> just one of them, and some comments about other aspects of the
>>>> warning.  Sorry if this seems like a lot.  I'm hoping you'll
>>>> find the feedback constructive.
>>>>
>>>> I don't think using void(*)(void) to suppress the warning is
>>>> a robust solution because it's not safe to call a function that
>>>> takes arguments through such a pointer (especially not if one
>>>> or more of the arguments is a pointer).  Depending on the ABI,
>>>> calling a function that expects arguments with none could also
>>>> mess up the stack as the callee may pop arguments that were
>>>> never passed to it.
>>>>
>>>
>>> This is of course only a heuristic, and if there is no warning
>>> that does not mean any guarantee that there can't be a problem
>>> at runtime.  The heuristic is only meant to separate the
>>> bad from the very bad type-cast.  In my personal opinion there
>>> is not a single good type cast.
>>
>> I agree.  Since the warning uses one kind of a cast as an escape
>> mechanism from the checking it should be one whose result can
>> the most likely be used to call the function without undefined
>> behavior.
>>
>> Since it's possible to call any function through a pointer to
>> a function with no arguments (simply by providing arguments of
>> matching types) it's a reasonable candidate.
>>
>> On the other hand, since it is not safe to call an arbitrary
>> function through void (*)(void), it's not as good a candidate.
>>
>> Another reason why I think a protoype-less function is a good
>> choice is because the alias and ifunc attributes already use it
>> as an escape mechanism from their type incompatibility warning.
>>
> 
> I know of pre-existing code-bases where a type-cast to type:
> void (*) (void);
> 
> .. is already used as a generic function pointer: libffi and
> libgo, I would not want to break these.
> 
> Actually when I have a type:
> X (*) (...);
> 
> I would like to make sure that the warning checks that
> only functions returning X are assigned.
> 
> and for X (*) (Y, ....);
> 
> I would like to check that anything returning X with
> first argument of type Y is assigned.
> 
> There are code bases where such a scheme is used.
> For instance one that I myself maintain: the OPC/UA AnsiC Stack,
> where I have this type definition:
> 
> typedef OpcUa_StatusCode (OpcUa_PfnInvokeService)(OpcUa_Endpoint
> hEndpoint, ...);
> 
> And this plays well together with this warning, because only
> functions are assigned that match up to the ...);
> Afterwards this pointer is cast back to the original signature,
> so everything is perfectly fine.
> 
> Regarding the cast from pointer to member to function, I see also a
> warning without -Wpedantic:
> Warnung: converting from »void (S::*)(int*)« to »void (*)(int*)«
> [-Wpmf-conversions]
>      F *pf = (F*)&S::foo;
>                      ^~~
> 
> And this one is even default-enabled, so I think that should be
> more than sufficient.
> 
> I also changed the heuristic, so that your example with the enum should
> now work.  I did not add it to the test case, because it would
> break with -fshort-enums :(
> 
> Attached I have an updated patch that extends this warning to the
> pointer-to-member function cast, and relaxes the heuristic on the
> benign integral type differences a bit further.
> 
> 
> Is it OK for trunk after bootstrap and reg-testing?
> 
> 
> Thanks
> Bernd.
>