Обсуждение: jsonb iterator not fully initialized

I got this error message via -fsanitized=undefined:

jsonfuncs.c:5169:12: runtime error: load of value 127, which is not a
valid value for type '_Bool'

The query was

select ts_headline('{}'::jsonb, tsquery('aaa & bbb'));

This calls the C function ts_headline_jsonb_byid_opt(), which calls
transform_jsonb_string_values(), which calls

    it = JsonbIteratorInit(&jsonb->root);
    is_scalar = it->isScalar;

but it->isScalar is not always initialized by JsonbIteratorInit().  (So
the 127 is quite likely clobbered memory.)

It can be fixed this way:

--- a/src/backend/utils/adt/jsonb_util.c
+++ b/src/backend/utils/adt/jsonb_util.c
@@ -901,7 +901,7 @@ iteratorFromContainer(JsonbContainer *container,
JsonbIterator *parent)
 {
    JsonbIterator *it;

-   it = palloc(sizeof(JsonbIterator));
+   it = palloc0(sizeof(JsonbIterator));
    it->container = container;
    it->parent = parent;
    it->nElems = JsonContainerSize(container);

It's probably not a problem in practice, since the isScalar business is
apparently only used in the array case, but it's dubious to leave things
uninitialized like this nonetheless.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-05-26 02:02, Peter Eisentraut wrote:
> I got this error message via -fsanitized=undefined:
> 
> jsonfuncs.c:5169:12: runtime error: load of value 127, which is not a
> valid value for type '_Bool'
> 
> The query was
> 
> select ts_headline('{}'::jsonb, tsquery('aaa & bbb'));
> 
> This calls the C function ts_headline_jsonb_byid_opt(), which calls
> transform_jsonb_string_values(), which calls
> 
>      it = JsonbIteratorInit(&jsonb->root);
>      is_scalar = it->isScalar;
> 
> but it->isScalar is not always initialized by JsonbIteratorInit().  (So
> the 127 is quite likely clobbered memory.)
> 
> It can be fixed this way:
> 
> --- a/src/backend/utils/adt/jsonb_util.c
> +++ b/src/backend/utils/adt/jsonb_util.c
> @@ -901,7 +901,7 @@ iteratorFromContainer(JsonbContainer *container,
> JsonbIterator *parent)
>   {
>      JsonbIterator *it;
> 
> -   it = palloc(sizeof(JsonbIterator));
> +   it = palloc0(sizeof(JsonbIterator));
>      it->container = container;
>      it->parent = parent;
>      it->nElems = JsonContainerSize(container);
> 
> It's probably not a problem in practice, since the isScalar business is
> apparently only used in the array case, but it's dubious to leave things
> uninitialized like this nonetheless.
> 

I've seen it earlier but couldn't decide what my proposed fix should 
look like. One of the options I considered was:

--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -5010,10 +5010,8 @@ transform_jsonb_string_values(Jsonb *jsonb, void 
*action_state,
         JsonbIteratorToken type;
         JsonbParseState *st = NULL;
         text       *out;
-       bool            is_scalar = false;

         it = JsonbIteratorInit(&jsonb->root);
-       is_scalar = it->isScalar;

         while ((type = JsonbIteratorNext(&it, &v, false)) != WJB_DONE)
         {
@@ -5033,7 +5031,7 @@ transform_jsonb_string_values(Jsonb *jsonb, void 
*action_state,
         }

         if (res->type == jbvArray)
-               res->val.array.rawScalar = is_scalar;
+               res->val.array.rawScalar = 
JsonContainerIsScalar(&jsonb->root);

         return JsonbValueToJsonb(res);
  }

On 05/26/2018 03:09 AM, Piotr Stefaniak wrote:
> On 2018-05-26 02:02, Peter Eisentraut wrote:
>> I got this error message via -fsanitized=undefined:
>>
>> jsonfuncs.c:5169:12: runtime error: load of value 127, which is not a
>> valid value for type '_Bool'
>>
>> The query was
>>
>> select ts_headline('{}'::jsonb, tsquery('aaa & bbb'));
>>
>> This calls the C function ts_headline_jsonb_byid_opt(), which calls
>> transform_jsonb_string_values(), which calls
>>
>>       it = JsonbIteratorInit(&jsonb->root);
>>       is_scalar = it->isScalar;
>>
>> but it->isScalar is not always initialized by JsonbIteratorInit().  (So
>> the 127 is quite likely clobbered memory.)
>>
>> It can be fixed this way:
>>
>> --- a/src/backend/utils/adt/jsonb_util.c
>> +++ b/src/backend/utils/adt/jsonb_util.c
>> @@ -901,7 +901,7 @@ iteratorFromContainer(JsonbContainer *container,
>> JsonbIterator *parent)
>>    {
>>       JsonbIterator *it;
>>
>> -   it = palloc(sizeof(JsonbIterator));
>> +   it = palloc0(sizeof(JsonbIterator));
>>       it->container = container;
>>       it->parent = parent;
>>       it->nElems = JsonContainerSize(container);
>>
>> It's probably not a problem in practice, since the isScalar business is
>> apparently only used in the array case, but it's dubious to leave things
>> uninitialized like this nonetheless.
>>
> I've seen it earlier but couldn't decide what my proposed fix should
> look like. One of the options I considered was:
>
> --- a/src/backend/utils/adt/jsonfuncs.c
> +++ b/src/backend/utils/adt/jsonfuncs.c
> @@ -5010,10 +5010,8 @@ transform_jsonb_string_values(Jsonb *jsonb, void
> *action_state,
>           JsonbIteratorToken type;
>           JsonbParseState *st = NULL;
>           text       *out;
> -       bool            is_scalar = false;
>
>           it = JsonbIteratorInit(&jsonb->root);
> -       is_scalar = it->isScalar;
>
>           while ((type = JsonbIteratorNext(&it, &v, false)) != WJB_DONE)
>           {
> @@ -5033,7 +5031,7 @@ transform_jsonb_string_values(Jsonb *jsonb, void
> *action_state,
>           }
>
>           if (res->type == jbvArray)
> -               res->val.array.rawScalar = is_scalar;
> +               res->val.array.rawScalar =
> JsonContainerIsScalar(&jsonb->root);
>
>           return JsonbValueToJsonb(res);
>    }


palloc0 seems cleaner.

cheers

andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> On 26 May 2018 at 16:47, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote:
> On 05/26/2018 03:09 AM, Piotr Stefaniak wrote:
>> On 2018-05-26 02:02, Peter Eisentraut wrote:
>>>
>>> It can be fixed this way:
>>>
>>> --- a/src/backend/utils/adt/jsonb_util.c
>>> +++ b/src/backend/utils/adt/jsonb_util.c
>>> @@ -901,7 +901,7 @@ iteratorFromContainer(JsonbContainer *container,
>>> JsonbIterator *parent)
>>>    {
>>>       JsonbIterator *it;
>>>
>>> -   it = palloc(sizeof(JsonbIterator));
>>> +   it = palloc0(sizeof(JsonbIterator));
>>>       it->container = container;
>>>       it->parent = parent;
>>>       it->nElems = JsonContainerSize(container);
>>>
>>> It's probably not a problem in practice, since the isScalar business is
>>> apparently only used in the array case, but it's dubious to leave things
>>> uninitialized like this nonetheless.

Yes, sounds reasonable.

>> I've seen it earlier but couldn't decide what my proposed fix should
>> look like. One of the options I considered was:
>>
>> --- a/src/backend/utils/adt/jsonfuncs.c
>> +++ b/src/backend/utils/adt/jsonfuncs.c
>> @@ -5010,10 +5010,8 @@ transform_jsonb_string_values(Jsonb *jsonb, void
>> *action_state,
>>           JsonbIteratorToken type;
>>           JsonbParseState *st = NULL;
>>           text       *out;
>> -       bool            is_scalar = false;
>>
>>           it = JsonbIteratorInit(&jsonb->root);
>> -       is_scalar = it->isScalar;
>>
>>           while ((type = JsonbIteratorNext(&it, &v, false)) != WJB_DONE)
>>           {
>> @@ -5033,7 +5031,7 @@ transform_jsonb_string_values(Jsonb *jsonb, void
>> *action_state,
>>           }
>>
>>           if (res->type == jbvArray)
>> -               res->val.array.rawScalar = is_scalar;
>> +               res->val.array.rawScalar =
>> JsonContainerIsScalar(&jsonb->root);
>>
>>           return JsonbValueToJsonb(res);
>>    }
>
> palloc0 seems cleaner.

Totally agree, palloc0 looks better (although I assume it's going to be
negligibly slower in those cases that aren't affected by this problem).