Обсуждение: [sqlsmith] Crash on GUC serialization

Hi,

sqlsmith just made a GUC that tricks the serialization code into
dereferencing a nullpointer.  Here's a recipe:

--8<---------------cut here---------------start------------->8---
set min_parallel_relation_size to 0;
set max_parallel_workers_per_gather to 2;
set force_parallel_mode to on;
begin;
select set_config('foo.baz', null, b) from (values (false), (true)) g(b);
commit;
select 1;
--8<---------------cut here---------------end--------------->8---

regards,
Andreas

Andreas Seltenreich <seltenreich@gmx.de> writes:
> sqlsmith just made a GUC that tricks the serialization code into
> dereferencing a nullpointer.  Here's a recipe:

Thanks for the report!  Looks like the serialization code has overlooked
the fact that string-valued GUCs can be NULL.  Surprising we didn't
find that before ...
        regards, tom lane

On Sat, Nov 19, 2016 at 9:31 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andreas Seltenreich <seltenreich@gmx.de> writes:
>> sqlsmith just made a GUC that tricks the serialization code into
>> dereferencing a nullpointer.  Here's a recipe:
>
> Thanks for the report!  Looks like the serialization code has overlooked
> the fact that string-valued GUCs can be NULL.  Surprising we didn't
> find that before ...

I was half-way through it when you sent your email. It seems to me
that we need to take care only of the case for PGC_STRING, per the
attached.
--
Michael

Michael Paquier writes:

> [2. text/plain; fix-guc-string-eval.patch]

I'm afraid taking care of the length computation is not sufficient.
ISTM like it'll still try to serialize the NULL pointer later on in
serialize_variable:

,----[ guc.c:9108 ]
| case PGC_STRING:
|     {
|     struct config_string *conf = (struct config_string *) gconf;
|     do_serialize(destptr, maxbytes, "%s", *conf->variable);
`----

On Sat, Nov 19, 2016 at 9:51 AM, Andreas Seltenreich <seltenreich@gmx.de> wrote:
> Michael Paquier writes:
>
>> [2. text/plain; fix-guc-string-eval.patch]
>
> I'm afraid taking care of the length computation is not sufficient.
> ISTM like it'll still try to serialize the NULL pointer later on in
> serialize_variable:
>
> ,----[ guc.c:9108 ]
> | case PGC_STRING:
> |     {
> |       struct config_string *conf = (struct config_string *) gconf;
> |       do_serialize(destptr, maxbytes, "%s", *conf->variable);
> `----

Hm, yes. Using an empty string strikes as being the best match.
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Sat, Nov 19, 2016 at 9:31 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Thanks for the report!  Looks like the serialization code has overlooked
>> the fact that string-valued GUCs can be NULL.  Surprising we didn't
>> find that before ...

> I was half-way through it when you sent your email. It seems to me
> that we need to take care only of the case for PGC_STRING, per the
> attached.

Meh; that might be stopping the crash for you but you didn't consider what
to do in serialize_variable.  Some machines would dump core there instead,
and others would print "(null)" which wouldn't fit in the allocated space.

The bigger problem here is that set_config_option cannot be used to set
a string GUC's value to NULL --- it will treat value == NULL as a request
to set to the reset_val.  The best we can do is to convert the value
into an empty string, which isn't quite the same thing, though it may be
close enough (compare e45e990e4).

I'm also pretty unexcited about serializing float variables this way ---
assuming that sprintf("%.17g") will reconstruct doubles exactly is
just asking for trouble IMO.

So I can't escape the itchy feeling that this entire chunk of code
needs to be thrown away and rewritten differently.  But it'd take
some refactoring of the code under set_config_option to do it nicely,
which is more work than we probably want to put in now.
        regards, tom lane

On Sat, Nov 19, 2016 at 12:31 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andreas Seltenreich <seltenreich@gmx.de> writes:
>> sqlsmith just made a GUC that tricks the serialization code into
>> dereferencing a nullpointer.  Here's a recipe:
>
> Thanks for the report!  Looks like the serialization code has overlooked
> the fact that string-valued GUCs can be NULL.  Surprising we didn't
> find that before ...

I had no idea that was possible.  I think if we'd had a regression
test that set a GUC to such a value, I would have caught this in the
raft of bug fixes I submitted around December of last year, so maybe
we should add one.  Why do we allow this, anyway?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Nov 19, 2016 at 12:31 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Thanks for the report!  Looks like the serialization code has overlooked
>> the fact that string-valued GUCs can be NULL.  Surprising we didn't
>> find that before ...

> Why do we allow this, anyway?

I think it simplifies some initialization cases.  Not sure how hard
it would be to remove that.  But this isn't the first such bug ...
        regards, tom lane