Обсуждение: Fix uninitialized variable access (src/backend/utils/mmgr/freepage.c)

On Fri, Jul 02, 2021 at 06:22:56PM -0300, Ranier Vilela wrote:
> Em qui., 1 de jul. de 2021 às 17:20, Mahendra Singh Thalor <
> mahi6run@gmail.com> escreveu:
>> Please can we try to hit this rare condition by any test case. If you have
>> any test cases, please share.

Yeah, this needs to be proved.  Are you sure that this change is
actually right?  The bottom of FreePageManagerPutInternal() has
assumptions that a page may not be found during a btree search, with
an index value used.
--
Michael

On Tue, Aug 17, 2021 at 9:13 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> If that's conditions happen, all *result.index* touches are garbage.
>

The patch looks valid to me, as the "index" member is not set in the
"btp == NULL" case, and so has a junk value in the caller, and it's
being used to index an array,
BUT - isn't it also necessary to set the "split_pages" member to 0,
because it also is not currently being set, and so too will have a
junk value in this case (and it's possible for it to be referenced by
the caller in this case).
The "btp == NULL" case is not hit by any existing test cases, and does
seem to be a rare case.


Regards,
Greg Nancarrow
Fujitsu Australia

At Tue, 17 Aug 2021 17:04:44 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> On Fri, Jul 02, 2021 at 06:22:56PM -0300, Ranier Vilela wrote:
> > Em qui., 1 de jul. de 2021 às 17:20, Mahendra Singh Thalor <
> > mahi6run@gmail.com> escreveu:
> >> Please can we try to hit this rare condition by any test case. If you have
> >> any test cases, please share.
>
> Yeah, this needs to be proved.  Are you sure that this change is
> actually right?  The bottom of FreePageManagerPutInternal() has
> assumptions that a page may not be found during a btree search, with
> an index value used.

By a quick look, FreePageBtreeSearch is called only from
FreePageManagerPutInternal at three points. The first one assumes that
result.found == true, at the rest points are passed only when
fpm->btree_depth > 0, i.e, fpm->btree_root is non-NULL.

In short FreePageBtreeSeach is never called when fpm->btree_root is
NULL.  I don't think we need to fill-in other members since the
contract of the function looks fine.

It might be simpler to turn 'if (btp == NULL)' to an assertion.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Aug 18, 2021 at 6:30 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> By a quick look, FreePageBtreeSearch is called only from
> FreePageManagerPutInternal at three points. The first one assumes that
> result.found == true, at the rest points are passed only when
> fpm->btree_depth > 0, i.e, fpm->btree_root is non-NULL.
>
> In short FreePageBtreeSeach is never called when fpm->btree_root is
> NULL.  I don't think we need to fill-in other members since the
> contract of the function looks fine.
>
> It might be simpler to turn 'if (btp == NULL)' to an assertion.
>

Even if there are no current calls to FreePageBtreeSeach() where it
results in "btp == NULL", FreePageBtreeSeach() is obviously handling
the possibility of that condition, and I think it's poor form to
return with two uninitialized members in the result for that,
especially when the current code for the "!result.found" case can
reference those members, and the usual return point of
FreePageBtreeSeach() has all result members set, including
result.found==true and result.found=false cases.
At best it's inconsistent and confusing and it looks like a bug
waiting to happen, so I'm still in favor of the patch.

Regards,
Greg Nancarrow
Fujitsu Australia

On 2021-08-18 1:29 a.m., Kyotaro Horiguchi wrote:
> At Tue, 17 Aug 2021 17:04:44 +0900, Michael Paquier <michael@paquier.xyz> wrote in
>> On Fri, Jul 02, 2021 at 06:22:56PM -0300, Ranier Vilela wrote:
>>> Em qui., 1 de jul. de 2021 às 17:20, Mahendra Singh Thalor <
>>> mahi6run@gmail.com> escreveu:
>>>> Please can we try to hit this rare condition by any test case. If you have
>>>> any test cases, please share.
>> Yeah, this needs to be proved.  Are you sure that this change is
>> actually right?  The bottom of FreePageManagerPutInternal() has
>> assumptions that a page may not be found during a btree search, with
>> an index value used.
> By a quick look, FreePageBtreeSearch is called only from
> FreePageManagerPutInternal at three points. The first one assumes that
> result.found == true, at the rest points are passed only when
> fpm->btree_depth > 0, i.e, fpm->btree_root is non-NULL.
>
> In short FreePageBtreeSeach is never called when fpm->btree_root is
> NULL.  I don't think we need to fill-in other members since the
> contract of the function looks fine.
>
> It might be simpler to turn 'if (btp == NULL)' to an assertion.
After added the initialization of split_pages in patch 
fix_unitialized_var_index_freepage-v1.patch,

+        result->split_pages = 0;

it actually changed the assertion condition after the second time 
function call of FreePageBtreeSearch.
             FreePageBtreeSearch(fpm, first_page, &result);

             /*
              * The act of allocating pages for use in constructing our 
btree
              * should never cause any page to become more full, so the new
              * split depth should be no greater than the old one, and 
perhaps
              * less if we fortuitously allocated a chunk that freed up 
a slot
              * on the page we need to update.
              */
             Assert(result.split_pages <= fpm->btree_recycle_count);

Should we consider adding some test cases to make sure this assertion 
will still function properly?

>
> regards.
>
-- 
David

Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca

On Fri, Oct 01, 2021 at 05:03:04PM -0300, Ranier Vilela wrote:
> For me the assertion remains valid and usable.

Well, I was looking at this thread again, and I still don't see what
we benefit from this change.  One thing that could also be done is to
initialize "result" at {0} at the top of FreePageManagerGetInternal()
and FreePageManagerPutInternal(), but that's in the same category as
the other suggestions.  I'll go drop the patch if there are no
objections.
--
Michael

On Thu, Jan 27, 2022 at 6:32 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Oct 01, 2021 at 05:03:04PM -0300, Ranier Vilela wrote:
> > For me the assertion remains valid and usable.
>
> Well, I was looking at this thread again, and I still don't see what
> we benefit from this change.  One thing that could also be done is to
> initialize "result" at {0} at the top of FreePageManagerGetInternal()
> and FreePageManagerPutInternal(), but that's in the same category as
> the other suggestions.  I'll go drop the patch if there are no
> objections.

Why not, at least, just add "Assert(result.page != NULL);" after the
"Assert(!result.found);" in FreePageManagerPutInternal()?
The following code block in FreePageBtreeSearch() - which lacks those
initializations - should never be invoked in this case, and the added
Assert will make this more obvious.

if (btp == NULL)
{
   result->page = NULL;
   result->found = false;
   return;
}

Regards,
Greg Nancarrow
Fujitsu Australia

On Thu, Jan 27, 2022 at 7:33 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
> Why not, at least, just add "Assert(result.page != NULL);" after the
> "Assert(!result.found);" in FreePageManagerPutInternal()?
> The following code block in FreePageBtreeSearch() - which lacks those
> initializations - should never be invoked in this case, and the added
> Assert will make this more obvious.
>
> if (btp == NULL)
> {
>    result->page = NULL;
>    result->found = false;
>    return;
> }

This patch is now in its fourth CommitFest, which is really a pretty
high number for a patch that has no demonstrated benefit. I'm marking
it rejected.

If you or someone else wants to submit a carefully-considered patch to
add meaningful assertions to this file in places where it would
clarify the intent of the code, please feel free to do that. But the
patch as presented doesn't do that. It simply initializes some
structure members to arbitrary values that probably won't produce
sensible results instead of leaving them uninitialized which probably
won't lead to sensible results either. It's been argued that this
could prevent future bugs, but I find that dubious. This code isn't
likely to be heavily modified in the future - it's a low-level
subsystem that has thus far shown no evidence of needing major
surgery. If surgery does happen in the future, I would argue that this
change could easily *mask* bugs, because if somebody tries to apply
valgrind to this code the useless initializations will just cover up
what valgrind would otherwise detect as an access to uninitialized
memory.

Please let's move on. There are almost 300 patches in this CommitFest
and many of them add nifty features or fix demonstrable bugs. This
does neither.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Mar 14, 2022 at 12:15 PM Robert Haas <robertmhaas@gmail.com> wrote:
> If surgery does happen in the future, I would argue that this
> change could easily *mask* bugs, because if somebody tries to apply
> valgrind to this code the useless initializations will just cover up
> what valgrind would otherwise detect as an access to uninitialized
> memory.

I agree. I have found it useful to VALGRIND_MAKE_MEM_DEFINED() a
buffer that would otherwise be considered initialized by Valgrind --
sometimes it's useful to make Valgrind understand that an area of
memory is garbage for all practical purposes. In other words,
sometimes it's useful to go out of your way to work around the problem
of meaningless initialization masking bugs (bugs that could otherwise
be detected by Valgrind).

Of course it's also possible that initializing memory to some
nominally safe value (e.g. using palloc0() rather than palloc()) makes
sense as a defensive measure. It depends on the specific code, of
course.

-- 
Peter Geoghegan