lparser.c:singlevar() asserts potentially uninitialised value.

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

lparser.c:singlevar() asserts potentially uninitialised value.

Duane Leslie
In lparser.c function `singlevar()` it checks that the `ls->envn`
named variable (i.e. _ENV) is initialised to a VLOCAL or VUPVAL, but
the `singlevaraux()` function may return VVOID without ever
initialising the `var->k` value.  The assert should be instead that
`singlevaraux()` doesn't return VVOID.

Regards,

Duane.

Reply | Threaded
Open this post in threaded view
|

Re: lparser.c:singlevar() asserts potentially uninitialised value.

Roberto Ierusalimschy
> In lparser.c function `singlevar()` it checks that the `ls->envn`
> named variable (i.e. _ENV) is initialised to a VLOCAL or VUPVAL, but
> the `singlevaraux()` function may return VVOID without ever
> initialising the `var->k` value.  The assert should be instead that
> `singlevaraux()` doesn't return VVOID.

We cannot put the call inside the assert, so we need to assign
the return to a variable and assert on that variable. But without
assertions, we get a "unused variable" warning (and a weird code).  Then
we add a "(void)var" to avoid the warning, and get weirder code.  All in
all, the current assertion documents what we want and keeps the regular
code simple. (After all, if the assertion holds, its code is correct ;-)

-- Roberto

Reply | Threaded
Open this post in threaded view
|

Re: lparser.c:singlevar() asserts potentially uninitialised value.

Duane Leslie
On 4 May 2016, at 22:58, Roberto Ierusalimschy <[hidden email]> wrote:

>> In lparser.c function `singlevar()` it checks that the `ls->envn`
>> named variable (i.e. _ENV) is initialised to a VLOCAL or VUPVAL, but
>> the `singlevaraux()` function may return VVOID without ever
>> initialising the `var->k` value.  The assert should be instead that
>> `singlevaraux()` doesn't return VVOID.
>
> We cannot put the call inside the assert, so we need to assign
> the return to a variable and assert on that variable. But without
> assertions, we get a "unused variable" warning (and a weird code).  Then
> we add a "(void)var" to avoid the warning, and get weirder code.  All in
> all, the current assertion documents what we want and keeps the regular
> code simple. (After all, if the assertion holds, its code is correct ;-)

You're claiming the converse of the assertion, not the contrapositive.  The actual statement should be that if the code is correct the assertion holds.   The problem is that assertions only state that if the assertion fails the code is invalid, they cannot assert correctness, especially in the case of undefined behaviour (in this case an uninitialised variable)

Avoiding a correct assertion for the sake of also avoiding "weird" code is a mistake in my opinion.  What is wrong a construction like:

```c
if (VVOID == singlevaraux()) {
  lua_assert(0 && "Error Message");
}
```

The problem is that your current assertion is misleading in what it documents because it implies that the `var->k` value is valid after the call to `singlevaraux`.

The assertion is particularly dangerous since it is actually quite likely to pass invalid code given the uninitialised value you are testing is on the stack.  If the code leads to the same call stack (highly likely) then you will exactly map your uninitialised value over the valid value used in a previous call stack.

Regards,

Duane.
Reply | Threaded
Open this post in threaded view
|

Re: lparser.c:singlevar() asserts potentially uninitialised value.

Duane Leslie
In reply to this post by Roberto Ierusalimschy
On 4 May 2016, at 22:58, Roberto Ierusalimschy <[hidden email]> wrote:

>> In lparser.c function `singlevar()` it checks that the `ls->envn`
>> named variable (i.e. _ENV) is initialised to a VLOCAL or VUPVAL, but
>> the `singlevaraux()` function may return VVOID without ever
>> initialising the `var->k` value.  The assert should be instead that
>> `singlevaraux()` doesn't return VVOID.
>
> We cannot put the call inside the assert, so we need to assign
> the return to a variable and assert on that variable. But without
> assertions, we get a "unused variable" warning (and a weird code).  Then
> we add a "(void)var" to avoid the warning, and get weirder code.  All in
> all, the current assertion documents what we want and keeps the regular
> code simple. (After all, if the assertion holds, its code is correct ;-)

You're claiming the converse of the assertion, not the contrapositive.  The actual statement should be that if the code is correct the assertion holds.   The problem is that assertions only state that if the assertion fails the code is invalid, they cannot assert correctness, especially in the case of undefined behaviour (in this case an uninitialised variable)

Avoiding a correct assertion for the sake of also avoiding "weird" code is a mistake in my opinion.  What is wrong a construction like:

```c
if (VVOID == singlevaraux()) {
 lua_assert(0 && "Error Message");
}
```

The problem is that your current assertion is misleading in what it documents because it implies that the `var->k` value is valid after the call to `singlevaraux`.

The assertion is particularly dangerous since it is actually quite likely to pass invalid code given the uninitialised value you are testing is on the stack.  If the code leads to the same call stack (highly likely) then you will exactly map your uninitialised value over the valid value used in a previous call stack.

Regards,

Duane
Reply | Threaded
Open this post in threaded view
|

Re: lparser.c:singlevar() asserts potentially uninitialised value.

Roberto Ierusalimschy
> > We cannot put the call inside the assert, so we need to assign
> > the return to a variable and assert on that variable. But without
> > assertions, we get a "unused variable" warning (and a weird code).  Then
> > we add a "(void)var" to avoid the warning, and get weirder code.  All in
> > all, the current assertion documents what we want and keeps the regular
> > code simple. (After all, if the assertion holds, its code is correct ;-)
>
> You're claiming the converse of the assertion, not the contrapositive.

Of course I am. The ";-)" in that message has a meaning...


> [...]  What is wrong a construction like:
>
> ```c
> if (VVOID == singlevaraux()) {
>  lua_assert(0 && "Error Message");
> }
> ```

It is (very) weird :-) It adds code outside an assertion only to make an
assertion. Some compiler could (with reason) give a warning about
these useless (and weird) comparison/test when assertions are off.

-- Roberto

Reply | Threaded
Open this post in threaded view
|

Re: lparser.c:singlevar() asserts potentially uninitialised value.

Roberto Ierusalimschy
> > > We cannot put the call inside the assert, so we need to assign
> > > the return to a variable and assert on that variable. But without
> > > assertions, we get a "unused variable" warning (and a weird code).  Then
> > > we add a "(void)var" to avoid the warning, and get weirder code.  All in
> > > all, the current assertion documents what we want and keeps the regular
> > > code simple. (After all, if the assertion holds, its code is correct ;-)
> >
> > You're claiming the converse of the assertion, not the contrapositive.
>
> Of course I am. The ";-)" in that message has a meaning...
>
>
> > [...]  What is wrong a construction like:
> >
> > ```c
> > if (VVOID == singlevaraux()) {
> >  lua_assert(0 && "Error Message");
> > }
> > ```
>
> It is (very) weird :-) It adds code outside an assertion only to make an
> assertion. Some compiler could (with reason) give a warning about
> these useless (and weird) comparison/test when assertions are off.

I guess the best solution is to rewrite 'singlevaraux' so that it
"returns" the kind of variable always in 'var->k'.

-- Roberto