Обсуждение: Fix uninitialized variable access (src/backend/utils/mmgr/freepage.c)
Hi,
The function FreePageManagerPutInternal can access an uninitialized variable,
if the following conditions occur:
1. fpm->btree_depth != 0
2. relptr_off == 0 inside function (FreePageBtreeSearch)
Perhaps this is a rare situation, but I think it's worth preventing.
/* Search the btree. */
FreePageBtreeSearch(fpm, first_page, &result);Assert(!result.found);
if (result.index > 0) /* result.index is garbage or invalid here) */
regards,
Ranier Vilela
Вложения
Re: Fix uninitialized variable access (src/backend/utils/mmgr/freepage.c)
От
Mahendra Singh Thalor
Дата:
On Fri, 2 Jul 2021 at 01:13, Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Hi,
>
> The function FreePageManagerPutInternal can access an uninitialized variable,
> if the following conditions occur:
Patch looks good to me.
> 1. fpm->btree_depth != 0
> 2. relptr_off == 0 inside function (FreePageBtreeSearch)
>
> Perhaps this is a rare situation, but I think it's worth preventing.
Please can we try to hit this rare condition by any test case. If you have any test cases, please share.
1064 FreePageBtreeSearch(FreePageManager *fpm, Size first_page,
1065 FreePageBtreeSearchResult *result)
1066 {
1067 char *base = fpm_segment_base(fpm);
1068 FreePageBtree *btp = relptr_access(base, fpm->btree_root);
1069 Size index;
1070
1071 result->split_pages = 1;
1072
1073 /* If the btree is empty, there's nothing to find. */
1074 if (btp == NULL)
1075 {
1076 result->page = NULL;
1077 result->found = false;
1078 return;
>
> /* Search the btree. */
> FreePageBtreeSearch(fpm, first_page, &result);
> Assert(!result.found);
> if (result.index > 0) /* result.index is garbage or invalid here) */
>
> regards,
> Ranier Vilela
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
>
> Hi,
>
> The function FreePageManagerPutInternal can access an uninitialized variable,
> if the following conditions occur:
Patch looks good to me.
> 1. fpm->btree_depth != 0
> 2. relptr_off == 0 inside function (FreePageBtreeSearch)
>
> Perhaps this is a rare situation, but I think it's worth preventing.
Please can we try to hit this rare condition by any test case. If you have any test cases, please share.
1064 FreePageBtreeSearch(FreePageManager *fpm, Size first_page,
1065 FreePageBtreeSearchResult *result)
1066 {
1067 char *base = fpm_segment_base(fpm);
1068 FreePageBtree *btp = relptr_access(base, fpm->btree_root);
1069 Size index;
1070
1071 result->split_pages = 1;
1072
1073 /* If the btree is empty, there's nothing to find. */
1074 if (btp == NULL)
1075 {
1076 result->page = NULL;
1077 result->found = false;
1078 return;
1079 }
> /* Search the btree. */
> FreePageBtreeSearch(fpm, first_page, &result);
> Assert(!result.found);
> if (result.index > 0) /* result.index is garbage or invalid here) */
>
> regards,
> Ranier Vilela
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
Em qui., 1 de jul. de 2021 às 17:20, Mahendra Singh Thalor <mahi6run@gmail.com> escreveu:
On Fri, 2 Jul 2021 at 01:13, Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Hi,
>
> The function FreePageManagerPutInternal can access an uninitialized variable,
> if the following conditions occur:
Patch looks good to me.
> 1. fpm->btree_depth != 0
> 2. relptr_off == 0 inside function (FreePageBtreeSearch)
>
> Perhaps this is a rare situation, but I think it's worth preventing.
Please can we try to hit this rare condition by any test case. If you have any test cases, please share.
Added to Commitfest (https://commitfest.postgresql.org/34/3236/), so we don't forget.
regards,
Ranier Vilela
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
Вложения
Em ter., 17 de ago. de 2021 às 05:04, Michael Paquier <michael@paquier.xyz> escreveu:
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.
Due to the absolute lack of reports, I believe that this particular case never happened.
Are you sure that this change is
actually right?
Yes, have.
The bottom of FreePageManagerPutInternal() has
assumptions that a page may not be found during a btree search, with
an index value used.
Assert assumptions are for Debug.
If that's conditions happen, all *result.index* touches are garbage.
regards,
Ranier Vilela
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
Em ter., 17 de ago. de 2021 às 10:22, Greg Nancarrow <gregn4422@gmail.com> escreveu:
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).
I agree.
Attached new version (v1).
regards,
Ranier Vilela
Вложения
Re: Fix uninitialized variable access (src/backend/utils/mmgr/freepage.c)
От
Kyotaro Horiguchi
Дата:
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
Em qua., 18 de ago. de 2021 às 05:30, Kyotaro Horiguchi <horikyota.ntt@gmail.com> escreveu:
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, it's a failure ready to happen, just someone who trusts FreePageBtreeSearch will do the right thing,
like not leaving structure with uninitialized fields.
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.
Quite the contrary, the contract is not being fulfilled.
It might be simpler to turn 'if (btp == NULL)' to an assertion.
Are you sure that no condition will ever occur in production?
Assertion is not for mistakes that can happen.
Assertion is not for mistakes that can happen.
regards,
Ranier Vilela
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
Em sex., 1 de out. de 2021 às 16:24, David Zhang <david.zhang@highgo.ca> escreveu:
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);
For me the assertion remains valid and usable.
regards,
Ranier Vilela
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