Обсуждение: [PATCH] Code refactoring related to -fsanitize=use-after-scope

Hello.

I've been currently working on support of -sanitize=use-after-scope in the GCC compiler and
I decided to use postgresql as my test-case. The sanitation poisons every stack variable at the
very beginning of a function, unpoisons a variable at the beginning of scope definition and finally
poisons the variable again at the end of scope.

Following patch fixes issues seen by the sanitizer. Hope it's acceptable?
With the patch applied, ASAN (with the new sanitization) works fine.

Thanks,
Martin

Hi,

On 2016-02-15 14:37:28 +0100, Martin Liška wrote:
> I've been currently working on support of -sanitize=use-after-scope in the GCC compiler and
> I decided to use postgresql as my test-case. The sanitation poisons every stack variable at the
> very beginning of a function, unpoisons a variable at the beginning of scope definition and finally
> poisons the variable again at the end of scope.

Generally sounds like a good check.

> Following patch fixes issues seen by the sanitizer. Hope it's acceptable?
> With the patch applied, ASAN (with the new sanitization) works fine.


> diff --git a/src/backend/access/spgist/spgdoinsert.c b/src/backend/access/spgist/spgdoinsert.c
> index f090ca5..ff986c2 100644
> --- a/src/backend/access/spgist/spgdoinsert.c
> +++ b/src/backend/access/spgist/spgdoinsert.c
> @@ -1871,6 +1871,10 @@ spgdoinsert(Relation index, SpGistState *state,
>      SPPageDesc    current,
>                  parent;
>      FmgrInfo   *procinfo = NULL;
> +    SpGistInnerTuple innerTuple;
> +    spgChooseIn in;
> +    spgChooseOut out;
> +
>  
>      /*
>       * Look up FmgrInfo of the user-defined choose function once, to save
> @@ -2044,9 +2048,6 @@ spgdoinsert(Relation index, SpGistState *state,
>               * Apply the opclass choose function to figure out how to insert
>               * the given datum into the current inner tuple.
>               */
> -            SpGistInnerTuple innerTuple;
> -            spgChooseIn in;
> -            spgChooseOut out;

But I'm not immediately seing why this is necessary? Is this about
battling a false positive?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2016-02-15 14:37:28 +0100, Martin Liška wrote:
>> I've been currently working on support of -sanitize=use-after-scope in the GCC compiler and
>> I decided to use postgresql as my test-case. The sanitation poisons every stack variable at the
>> very beginning of a function, unpoisons a variable at the beginning of scope definition and finally
>> poisons the variable again at the end of scope.

> Generally sounds like a good check.

>> Following patch fixes issues seen by the sanitizer. Hope it's acceptable?
>> With the patch applied, ASAN (with the new sanitization) works fine.

> But I'm not immediately seing why this is necessary? Is this about
> battling a false positive?

I bet a nickel that this is triggered by the goto leading into those
variables' scope ("goto process_inner_tuple" at line 2038 in HEAD).
That probably bypasses the "unpoison" step.

However, doesn't this represent a bug in the sanitizer rather than
anything we should change in Postgres?  There is no rule in C that
you can't execute such a goto, especially not if there is no
initialization of those variables.

If you can think of a reasonable refactoring that gets rid of the need
for that goto, I'd be for that, because it's certainly unsightly.
But I don't think it's wrong, and I don't think that the proposed patch
is any improvement from a structured-programming standpoint.
        regards, tom lane

On 02/15/2016 08:20 PM, Tom Lane wrote:
> I bet a nickel that this is triggered by the goto leading into those
> variables' scope ("goto process_inner_tuple" at line 2038 in HEAD).
> That probably bypasses the "unpoison" step.
> 
> However, doesn't this represent a bug in the sanitizer rather than
> anything we should change in Postgres?  There is no rule in C that
> you can't execute such a goto, especially not if there is no
> initialization of those variables.
> 
> If you can think of a reasonable refactoring that gets rid of the need
> for that goto, I'd be for that, because it's certainly unsightly.
> But I don't think it's wrong, and I don't think that the proposed patch
> is any improvement from a structured-programming standpoint.
> 
>             regards, tom lane

Hi Tom.

You are exactly right that as the code does not expose an initialization,
it should work fine. As you mentioned, unpoisoning is skipped that exposes
this false positive.

I'll try to think about the case and handle that. Application of my patch
does not make sense.

Martin