Quiteing clang static analyzer

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Quiteing clang static analyzer

Jerry Jacobs
I’m using lua 5.3 in a project and clang static analyzer (scan-build) complained about a potential NULL pointer deference in OS lib (loslib.c). The analyzer doesn’t know luaL_error uses setjmp/longjmp and it has its full right to complain. The os_date C function contains this code:

stm = l_gmtime(&t, &tmr);

~~~ snip ~~~

  if (stm == NULL)  /* invalid date? */
    luaL_error(L, "time result cannot be represented in this installation”);

~~~ snip ~~~

problem “passing NULL pointer” ->  setallfields(L, stm);

A simple solution would be just doing a return luaL_error which silences this false-positive.

Kind regards,
Jerry Jacobs
Reply | Threaded
Open this post in threaded view
|

Re: Quiteing clang static analyzer

Igor Ehrlich
I believe the proper way is to mark luaL_error with a no-return specifier? Something like GCC's "__attribute__((noreturn))" would possibly do the trick. Also, this the actual syntactic expression for "functions using setjmp/longjmp".

On Mon, Mar 13, 2017 at 11:54 PM, Jerry Jacobs <[hidden email]> wrote:
I’m using lua 5.3 in a project and clang static analyzer (scan-build) complained about a potential NULL pointer deference in OS lib (loslib.c). The analyzer doesn’t know luaL_error uses setjmp/longjmp and it has its full right to complain. The os_date C function contains this code:

stm = l_gmtime(&t, &tmr);

~~~ snip ~~~

  if (stm == NULL)  /* invalid date? */
    luaL_error(L, "time result cannot be represented in this installation”);

~~~ snip ~~~

problem “passing NULL pointer” ->  setallfields(L, stm);

A simple solution would be just doing a return luaL_error which silences this false-positive.

Kind regards,
Jerry Jacobs



--
Best regards,
Igor A. Ehrlich
Reply | Threaded
Open this post in threaded view
|

Re: Quiteing clang static analyzer

Jerry Jacobs
It hasn’t anything todo with the incorrectness of luaL_error. But with the analyzer choking on a potential NULL pointer deference. The function os_date returns an int. So does luaL_error return an int. On other places return luaL_error(“”) is implemented, except for this place. Which would be:

  if (stm == NULL)  /* invalid date? *
    return luaL_error(L, "time result cannot be represented in this installation”);

Its only necessary for the analyzer to shut its mouth, the code is 100% correct.

> On 13 Mar 2017, at 22:25, Igor Ehrlich <[hidden email]> wrote:
>
> I believe the proper way is to mark luaL_error with a no-return specifier? Something like GCC's "__attribute__((noreturn))" would possibly do the trick. Also, this the actual syntactic expression for "functions using setjmp/longjmp".
>
> On Mon, Mar 13, 2017 at 11:54 PM, Jerry Jacobs <[hidden email]> wrote:
> I’m using lua 5.3 in a project and clang static analyzer (scan-build) complained about a potential NULL pointer deference in OS lib (loslib.c). The analyzer doesn’t know luaL_error uses setjmp/longjmp and it has its full right to complain. The os_date C function contains this code:
>
> stm = l_gmtime(&t, &tmr);
>
> ~~~ snip ~~~
>
>   if (stm == NULL)  /* invalid date? */
>     luaL_error(L, "time result cannot be represented in this installation”);
>
> ~~~ snip ~~~
>
> problem “passing NULL pointer” ->  setallfields(L, stm);
>
> A simple solution would be just doing a return luaL_error which silences this false-positive.
>
> Kind regards,
> Jerry Jacobs
>
>
>
> --
> Best regards,
> Igor A. Ehrlich


Reply | Threaded
Open this post in threaded view
|

Re: Quiteing clang static analyzer

Igor Ehrlich
I'll just cautiously metion that the analyzer warning is 100% correct from the language perspective. Should we possibly consider fixing it in the interpreter? The fix seems to be _rather_ straightforward, without any possibly negative performance effects.

On Tue, Mar 14, 2017 at 12:32 AM, Jerry Jacobs <[hidden email]> wrote:
It hasn’t anything todo with the incorrectness of luaL_error. But with the analyzer choking on a potential NULL pointer deference. The function os_date returns an int. So does luaL_error return an int. On other places return luaL_error(“”) is implemented, except for this place. Which would be:

  if (stm == NULL)  /* invalid date? *
    return luaL_error(L, "time result cannot be represented in this installation”);

Its only necessary for the analyzer to shut its mouth, the code is 100% correct.

> On 13 Mar 2017, at 22:25, Igor Ehrlich <[hidden email]> wrote:
>
> I believe the proper way is to mark luaL_error with a no-return specifier? Something like GCC's "__attribute__((noreturn))" would possibly do the trick. Also, this the actual syntactic expression for "functions using setjmp/longjmp".
>
> On Mon, Mar 13, 2017 at 11:54 PM, Jerry Jacobs <[hidden email]> wrote:
> I’m using lua 5.3 in a project and clang static analyzer (scan-build) complained about a potential NULL pointer deference in OS lib (loslib.c). The analyzer doesn’t know luaL_error uses setjmp/longjmp and it has its full right to complain. The os_date C function contains this code:
>
> stm = l_gmtime(&t, &tmr);
>
> ~~~ snip ~~~
>
>   if (stm == NULL)  /* invalid date? */
>     luaL_error(L, "time result cannot be represented in this installation”);
>
> ~~~ snip ~~~
>
> problem “passing NULL pointer” ->  setallfields(L, stm);
>
> A simple solution would be just doing a return luaL_error which silences this false-positive.
>
> Kind regards,
> Jerry Jacobs
>
>
>
> --
> Best regards,
> Igor A. Ehrlich





--
Best regards,
Igor A. Ehrlich
Reply | Threaded
Open this post in threaded view
|

Re: Quiteing clang static analyzer

Igor Ehrlich
I actually understand why it was not done. That's a pity, sorry for the noise.

On Tue, Mar 14, 2017 at 12:43 AM, Igor Ehrlich <[hidden email]> wrote:
I'll just cautiously metion that the analyzer warning is 100% correct from the language perspective. Should we possibly consider fixing it in the interpreter? The fix seems to be _rather_ straightforward, without any possibly negative performance effects.

On Tue, Mar 14, 2017 at 12:32 AM, Jerry Jacobs <[hidden email]> wrote:
It hasn’t anything todo with the incorrectness of luaL_error. But with the analyzer choking on a potential NULL pointer deference. The function os_date returns an int. So does luaL_error return an int. On other places return luaL_error(“”) is implemented, except for this place. Which would be:

  if (stm == NULL)  /* invalid date? *
    return luaL_error(L, "time result cannot be represented in this installation”);

Its only necessary for the analyzer to shut its mouth, the code is 100% correct.

> On 13 Mar 2017, at 22:25, Igor Ehrlich <[hidden email]> wrote:
>
> I believe the proper way is to mark luaL_error with a no-return specifier? Something like GCC's "__attribute__((noreturn))" would possibly do the trick. Also, this the actual syntactic expression for "functions using setjmp/longjmp".
>
> On Mon, Mar 13, 2017 at 11:54 PM, Jerry Jacobs <[hidden email]> wrote:
> I’m using lua 5.3 in a project and clang static analyzer (scan-build) complained about a potential NULL pointer deference in OS lib (loslib.c). The analyzer doesn’t know luaL_error uses setjmp/longjmp and it has its full right to complain. The os_date C function contains this code:
>
> stm = l_gmtime(&t, &tmr);
>
> ~~~ snip ~~~
>
>   if (stm == NULL)  /* invalid date? */
>     luaL_error(L, "time result cannot be represented in this installation”);
>
> ~~~ snip ~~~
>
> problem “passing NULL pointer” ->  setallfields(L, stm);
>
> A simple solution would be just doing a return luaL_error which silences this false-positive.
>
> Kind regards,
> Jerry Jacobs
>
>
>
> --
> Best regards,
> Igor A. Ehrlich





--
Best regards,
Igor A. Ehrlich



--
Best regards,
Igor A. Ehrlich
Reply | Threaded
Open this post in threaded view
|

Re: Quiteing clang static analyzer

Nagaev Boris
Can you share why it was not done?

Adding __attribute__((noreturn)) to the one function seems better than
adding forgotten returns to all the invocations. The only thing I am
concerned about is a phrase from the manual:

> it is an idiom to use it in C functions as return luaL_error(args).

which this code does not follow.

On Mon, Mar 13, 2017 at 9:48 PM, Igor Ehrlich <[hidden email]> wrote:

> I actually understand why it was not done. That's a pity, sorry for the
> noise.
>
> On Tue, Mar 14, 2017 at 12:43 AM, Igor Ehrlich <[hidden email]> wrote:
>>
>> I'll just cautiously metion that the analyzer warning is 100% correct from
>> the language perspective. Should we possibly consider fixing it in the
>> interpreter? The fix seems to be _rather_ straightforward, without any
>> possibly negative performance effects.
>>
>> On Tue, Mar 14, 2017 at 12:32 AM, Jerry Jacobs <[hidden email]>
>> wrote:
>>>
>>> It hasn’t anything todo with the incorrectness of luaL_error. But with
>>> the analyzer choking on a potential NULL pointer deference. The function
>>> os_date returns an int. So does luaL_error return an int. On other places
>>> return luaL_error(“”) is implemented, except for this place. Which would be:
>>>
>>>   if (stm == NULL)  /* invalid date? *
>>>     return luaL_error(L, "time result cannot be represented in this
>>> installation”);
>>>
>>> Its only necessary for the analyzer to shut its mouth, the code is 100%
>>> correct.
>>>
>>> > On 13 Mar 2017, at 22:25, Igor Ehrlich <[hidden email]> wrote:
>>> >
>>> > I believe the proper way is to mark luaL_error with a no-return
>>> > specifier? Something like GCC's "__attribute__((noreturn))" would possibly
>>> > do the trick. Also, this the actual syntactic expression for "functions
>>> > using setjmp/longjmp".
>>> >
>>> > On Mon, Mar 13, 2017 at 11:54 PM, Jerry Jacobs
>>> > <[hidden email]> wrote:
>>> > I’m using lua 5.3 in a project and clang static analyzer (scan-build)
>>> > complained about a potential NULL pointer deference in OS lib (loslib.c).
>>> > The analyzer doesn’t know luaL_error uses setjmp/longjmp and it has its full
>>> > right to complain. The os_date C function contains this code:
>>> >
>>> > stm = l_gmtime(&t, &tmr);
>>> >
>>> > ~~~ snip ~~~
>>> >
>>> >   if (stm == NULL)  /* invalid date? */
>>> >     luaL_error(L, "time result cannot be represented in this
>>> > installation”);
>>> >
>>> > ~~~ snip ~~~
>>> >
>>> > problem “passing NULL pointer” ->  setallfields(L, stm);
>>> >
>>> > A simple solution would be just doing a return luaL_error which
>>> > silences this false-positive.
>>> >
>>> > Kind regards,
>>> > Jerry Jacobs
>>> >
>>> >
>>> >
>>> > --
>>> > Best regards,
>>> > Igor A. Ehrlich
>>>
>>>
>>
>>
>>
>> --
>> Best regards,
>> Igor A. Ehrlich
>
>
>
>
> --
> Best regards,
> Igor A. Ehrlich



--
Best regards,
Boris Nagaev

Reply | Threaded
Open this post in threaded view
|

Re: Quiteing clang static analyzer

Jerry Jacobs

> On 14 Mar 2017, at 20:44, Nagaev Boris <[hidden email]> wrote:
>
> Can you share why it was not done?
>
> Adding __attribute__((noreturn)) to the one function seems better than
> adding forgotten returns to all the invocations. The only thing I am
> concerned about is a phrase from the manual:

Adding noreturn doesn’t resolve the issue I’m talking about. Keep in mind
 attributes are compiler specific. It is possible some compilers will not support
 them. A static analyzer doesn’t have to implement all compiler features which
 are not in the standard.

>
>> it is an idiom to use it in C functions as return luaL_error(args).
>
> which this code does not follow.
>
> On Mon, Mar 13, 2017 at 9:48 PM, Igor Ehrlich <[hidden email]> wrote:
>> I actually understand why it was not done. That's a pity, sorry for the
>> noise.
>>
>> On Tue, Mar 14, 2017 at 12:43 AM, Igor Ehrlich <[hidden email]> wrote:
>>>
>>> I'll just cautiously metion that the analyzer warning is 100% correct from
>>> the language perspective. Should we possibly consider fixing it in the
>>> interpreter? The fix seems to be _rather_ straightforward, without any
>>> possibly negative performance effects.
>>>
>>> On Tue, Mar 14, 2017 at 12:32 AM, Jerry Jacobs <[hidden email]>
>>> wrote:
>>>>
>>>> It hasn’t anything todo with the incorrectness of luaL_error. But with
>>>> the analyzer choking on a potential NULL pointer deference. The function
>>>> os_date returns an int. So does luaL_error return an int. On other places
>>>> return luaL_error(“”) is implemented, except for this place. Which would be:
>>>>
>>>>  if (stm == NULL)  /* invalid date? *
>>>>    return luaL_error(L, "time result cannot be represented in this
>>>> installation”);
>>>>
>>>> Its only necessary for the analyzer to shut its mouth, the code is 100%
>>>> correct.
>>>>
>>>>> On 13 Mar 2017, at 22:25, Igor Ehrlich <[hidden email]> wrote:
>>>>>
>>>>> I believe the proper way is to mark luaL_error with a no-return
>>>>> specifier? Something like GCC's "__attribute__((noreturn))" would possibly
>>>>> do the trick. Also, this the actual syntactic expression for "functions
>>>>> using setjmp/longjmp".
>>>>>
>>>>> On Mon, Mar 13, 2017 at 11:54 PM, Jerry Jacobs
>>>>> <[hidden email]> wrote:
>>>>> I’m using lua 5.3 in a project and clang static analyzer (scan-build)
>>>>> complained about a potential NULL pointer deference in OS lib (loslib.c).
>>>>> The analyzer doesn’t know luaL_error uses setjmp/longjmp and it has its full
>>>>> right to complain. The os_date C function contains this code:
>>>>>
>>>>> stm = l_gmtime(&t, &tmr);
>>>>>
>>>>> ~~~ snip ~~~
>>>>>
>>>>>  if (stm == NULL)  /* invalid date? */
>>>>>    luaL_error(L, "time result cannot be represented in this
>>>>> installation”);
>>>>>
>>>>> ~~~ snip ~~~
>>>>>
>>>>> problem “passing NULL pointer” ->  setallfields(L, stm);
>>>>>
>>>>> A simple solution would be just doing a return luaL_error which
>>>>> silences this false-positive.
>>>>>
>>>>> Kind regards,
>>>>> Jerry Jacobs
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Best regards,
>>>>> Igor A. Ehrlich
>>>>
>>>>
>>>
>>>
>>>
>>> --
>>> Best regards,
>>> Igor A. Ehrlich
>>
>>
>>
>>
>> --
>> Best regards,
>> Igor A. Ehrlich
>
>
>
> --
> Best regards,
> Boris Nagaev
>


Reply | Threaded
Open this post in threaded view
|

Re: Quiteing clang static analyzer

Igor Ehrlich
> Can you share why it was not done?
>
> Adding __attribute__((noreturn)) to the one function seems better than
> adding forgotten returns to all the invocations. The only thing I am
> concerned about is a phrase from the manual:

> > it is an idiom to use it in C functions as return luaL_error(args).

> which this code does not follow.

Well, my words might be misleading, I rather understand why I would not do this :)

Note, that twice-underlying "luaG_errormsg" actually carries "l_noret" specifier. The reason why "luaL_error" does not carry it - if you'd want it to work, you'd have to provide compiler-dependent definition of "l_noret" (or similar construct) alongside with "lua.h". And even if you do this, there might always be a compiler who was not invited to the party, in this case "l_noret" will still be ignored and the desired behaviour will not be reached. This is true for both surrounding code and for the platform code that uses "luaL_error". That's why this idiom you mentioned is used - it allows to break data flow on the current control path, just as "__attribute__((noreturn))" does.

Also, note how this idiom is omitted in "lua_error" :)

One possible option is to add something like

> #ifndef LUA_NORET
> #define LUA_NORET void
> #endif
> ...
> LUA_NORET luaL_error(...
>

and allow the surrounding code to re-define LUA_NORET according to its needs. And if you do this, you'll have to worry about the linker possibly going insane between lua library compiled by gcc and the client code compiled by something else. Because I highly doubt that all compilers handle noreturn-mangled symbols equally. This point also stands for my initial idea of pre-shipping "l_noret" token, if Lua and client are compiled with different compilers.

All in all, this idiom is the most harmless design solution I see so far. So yes, it works for me. 

P.S.: Also, creating "l_noret" internal counterpart of luaL_error would solve the initial issue.

On Tue, Mar 14, 2017 at 10:48 PM, Jerry Jacobs <[hidden email]> wrote:

> On 14 Mar 2017, at 20:44, Nagaev Boris <[hidden email]> wrote:
>
> Can you share why it was not done?
>
> Adding __attribute__((noreturn)) to the one function seems better than
> adding forgotten returns to all the invocations. The only thing I am
> concerned about is a phrase from the manual:

Adding noreturn doesn’t resolve the issue I’m talking about. Keep in mind
 attributes are compiler specific. It is possible some compilers will not support
 them. A static analyzer doesn’t have to implement all compiler features which
 are not in the standard.

>
>> it is an idiom to use it in C functions as return luaL_error(args).
>
> which this code does not follow.
>
> On Mon, Mar 13, 2017 at 9:48 PM, Igor Ehrlich <[hidden email]> wrote:
>> I actually understand why it was not done. That's a pity, sorry for the
>> noise.
>>
>> On Tue, Mar 14, 2017 at 12:43 AM, Igor Ehrlich <[hidden email]> wrote:
>>>
>>> I'll just cautiously metion that the analyzer warning is 100% correct from
>>> the language perspective. Should we possibly consider fixing it in the
>>> interpreter? The fix seems to be _rather_ straightforward, without any
>>> possibly negative performance effects.
>>>
>>> On Tue, Mar 14, 2017 at 12:32 AM, Jerry Jacobs <[hidden email]>
>>> wrote:
>>>>
>>>> It hasn’t anything todo with the incorrectness of luaL_error. But with
>>>> the analyzer choking on a potential NULL pointer deference. The function
>>>> os_date returns an int. So does luaL_error return an int. On other places
>>>> return luaL_error(“”) is implemented, except for this place. Which would be:
>>>>
>>>>  if (stm == NULL)  /* invalid date? *
>>>>    return luaL_error(L, "time result cannot be represented in this
>>>> installation”);
>>>>
>>>> Its only necessary for the analyzer to shut its mouth, the code is 100%
>>>> correct.
>>>>
>>>>> On 13 Mar 2017, at 22:25, Igor Ehrlich <[hidden email]> wrote:
>>>>>
>>>>> I believe the proper way is to mark luaL_error with a no-return
>>>>> specifier? Something like GCC's "__attribute__((noreturn))" would possibly
>>>>> do the trick. Also, this the actual syntactic expression for "functions
>>>>> using setjmp/longjmp".
>>>>>
>>>>> On Mon, Mar 13, 2017 at 11:54 PM, Jerry Jacobs
>>>>> <[hidden email]> wrote:
>>>>> I’m using lua 5.3 in a project and clang static analyzer (scan-build)
>>>>> complained about a potential NULL pointer deference in OS lib (loslib.c).
>>>>> The analyzer doesn’t know luaL_error uses setjmp/longjmp and it has its full
>>>>> right to complain. The os_date C function contains this code:
>>>>>
>>>>> stm = l_gmtime(&t, &tmr);
>>>>>
>>>>> ~~~ snip ~~~
>>>>>
>>>>>  if (stm == NULL)  /* invalid date? */
>>>>>    luaL_error(L, "time result cannot be represented in this
>>>>> installation”);
>>>>>
>>>>> ~~~ snip ~~~
>>>>>
>>>>> problem “passing NULL pointer” ->  setallfields(L, stm);
>>>>>
>>>>> A simple solution would be just doing a return luaL_error which
>>>>> silences this false-positive.
>>>>>
>>>>> Kind regards,
>>>>> Jerry Jacobs
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Best regards,
>>>>> Igor A. Ehrlich
>>>>
>>>>
>>>
>>>
>>>
>>> --
>>> Best regards,
>>> Igor A. Ehrlich
>>
>>
>>
>>
>> --
>> Best regards,
>> Igor A. Ehrlich
>
>
>
> --
> Best regards,
> Boris Nagaev
>





--
Best regards,
Igor A. Ehrlich
Reply | Threaded
Open this post in threaded view
|

Re: Quiteing clang static analyzer

Igor Ehrlich
In reply to this post by Nagaev Boris
Also, that's the actual benefit of the continuous usage of static analyzers :)

On Tue, Mar 14, 2017 at 10:44 PM, Nagaev Boris <[hidden email]> wrote:
Can you share why it was not done?

Adding __attribute__((noreturn)) to the one function seems better than
adding forgotten returns to all the invocations. The only thing I am
concerned about is a phrase from the manual:

> it is an idiom to use it in C functions as return luaL_error(args).

which this code does not follow.

On Mon, Mar 13, 2017 at 9:48 PM, Igor Ehrlich <[hidden email]> wrote:
> I actually understand why it was not done. That's a pity, sorry for the
> noise.
>
> On Tue, Mar 14, 2017 at 12:43 AM, Igor Ehrlich <[hidden email]> wrote:
>>
>> I'll just cautiously metion that the analyzer warning is 100% correct from
>> the language perspective. Should we possibly consider fixing it in the
>> interpreter? The fix seems to be _rather_ straightforward, without any
>> possibly negative performance effects.
>>
>> On Tue, Mar 14, 2017 at 12:32 AM, Jerry Jacobs <[hidden email]>
>> wrote:
>>>
>>> It hasn’t anything todo with the incorrectness of luaL_error. But with
>>> the analyzer choking on a potential NULL pointer deference. The function
>>> os_date returns an int. So does luaL_error return an int. On other places
>>> return luaL_error(“”) is implemented, except for this place. Which would be:
>>>
>>>   if (stm == NULL)  /* invalid date? *
>>>     return luaL_error(L, "time result cannot be represented in this
>>> installation”);
>>>
>>> Its only necessary for the analyzer to shut its mouth, the code is 100%
>>> correct.
>>>
>>> > On 13 Mar 2017, at 22:25, Igor Ehrlich <[hidden email]> wrote:
>>> >
>>> > I believe the proper way is to mark luaL_error with a no-return
>>> > specifier? Something like GCC's "__attribute__((noreturn))" would possibly
>>> > do the trick. Also, this the actual syntactic expression for "functions
>>> > using setjmp/longjmp".
>>> >
>>> > On Mon, Mar 13, 2017 at 11:54 PM, Jerry Jacobs
>>> > <[hidden email]> wrote:
>>> > I’m using lua 5.3 in a project and clang static analyzer (scan-build)
>>> > complained about a potential NULL pointer deference in OS lib (loslib.c).
>>> > The analyzer doesn’t know luaL_error uses setjmp/longjmp and it has its full
>>> > right to complain. The os_date C function contains this code:
>>> >
>>> > stm = l_gmtime(&t, &tmr);
>>> >
>>> > ~~~ snip ~~~
>>> >
>>> >   if (stm == NULL)  /* invalid date? */
>>> >     luaL_error(L, "time result cannot be represented in this
>>> > installation”);
>>> >
>>> > ~~~ snip ~~~
>>> >
>>> > problem “passing NULL pointer” ->  setallfields(L, stm);
>>> >
>>> > A simple solution would be just doing a return luaL_error which
>>> > silences this false-positive.
>>> >
>>> > Kind regards,
>>> > Jerry Jacobs
>>> >
>>> >
>>> >
>>> > --
>>> > Best regards,
>>> > Igor A. Ehrlich
>>>
>>>
>>
>>
>>
>> --
>> Best regards,
>> Igor A. Ehrlich
>
>
>
>
> --
> Best regards,
> Igor A. Ehrlich



--
Best regards,
Boris Nagaev




--
Best regards,
Igor A. Ehrlich