Re: Creating a function for exposing memory usage of backend process

On 2020/08/17 21:14, Fujii Masao wrote:
>
>
> On 2020/08/11 15:24, torikoshia wrote:
>> On 2020-08-08 10:44, Michael Paquier wrote:
>>> On Fri, Jul 31, 2020 at 03:23:52PM -0400, Robert Haas wrote:
>>>> On Fri, Jul 31, 2020 at 4:25 AM torikoshia <torikoshia(at)oss(dot)nttdata(dot)com> wrote:
>>>>> And as Fujii-san told me in person, exposing memory address seems
>>>>> not preferable considering there are security techniques like
>>>>> address space layout randomization.
>>>>
>>>> Yeah, exactly. ASLR wouldn't do anything to improve security if there
>>>> were no other security bugs, but there are, and some of those bugs are
>>>> harder to exploit if you don't know the precise memory addresses of
>>>> certain data structures. Similarly, exposing the addresses of our
>>>> internal data structures is harmless if we have no other security
>>>> bugs, but if we do, it might make those bugs easier to exploit. I
>>>> don't think this information is useful enough to justify taking that
>>>> risk.
>>>
>>> FWIW, this is the class of issues where it is possible to print some
>>> areas of memory, or even manipulate the stack so as it was possible to
>>> pass down a custom pointer, so exposing the pointer locations is a
>>> real risk, and this has happened in the past.  Anyway, it seems to me
>>> that if this part is done, we could just make it superuser-only with
>>> restrictive REVOKE privileges, but I am not sure that we have enough
>>> user cases to justify this addition.
>>
>>
>> Thanks for your comments!
>>
>> I convinced that exposing pointer locations introduce security risks
>> and it seems better not to do so.
>>
>> And I now feel identifying exact memory context by exposing memory
>> address or other means seems overkill.
>> Showing just the context name of the parent would be sufficient and
>> 0007 pattch takes this way.
>
> Agreed.
>
>
>>
>>
>> On 2020-08-07 16:38, Kasahara Tatsuhito wrote:
>>> The following review has been posted through the commitfest application:
>>> make installcheck-world:  tested, passed
>>> Implements feature:       tested, passed
>>> Spec compliant:           not tested
>>> Documentation:            tested, passed
>>>
>>> I tested the latest
>>> patch(0007-Adding-a-function-exposing-memory-usage-of-local-backend.patch)
>>> with the latest PG-version (199cec9779504c08aaa8159c6308283156547409)
>>> and test was passed.
>>> It looks good to me.
>>>
>>> The new status of this patch is: Ready for Committer
>>
>> Thanks for your testing!
>
> Thanks for updating the patch! Here are the review comments.
>
> +     <row>
> +      <entry><link linkend="view-pg-backend-memory-contexts"><structname>pg_backend_memory_contexts</structname></link></entry>
> +      <entry>backend memory contexts</entry>
> +     </row>
>
> The above is located just after pg_matviews entry. But it should be located
> just after pg_available_extension_versions entry. Because the rows in the table
> "System Views" should be located in alphabetical order.
>
>
> + <sect1 id="view-pg-backend-memory-contexts">
> +  <title><structname>pg_backend_memory_contexts</structname></title>
>
> Same as above.
>
>
> +   The view <structname>pg_backend_memory_contexts</structname> displays all
> +   the local backend memory contexts.
>
> This description seems a bit confusing because maybe we can interpret this
> as "... displays the memory contexts of all the local backends" wrongly. Thought?
> What about the following description, instead?
>
>     The view <structname>pg_backend_memory_contexts</structname> displays all
>     the memory contexts of the server process attached to the current session.
>
>
> +    const char *name = context->name;
> +    const char *ident = context->ident;
> +
> +    if (context == NULL)
> +        return;
>
> The above check "context == NULL" is useless? If "context" is actually NULL,
> "context->name" would cause segmentation fault, so ISTM that the check
> will never be performed.
>
> If "context" can be NULL, the check should be performed before accessing
> to "contect". OTOH, if "context" must not be NULL per the specification of
> PutMemoryContextStatsTupleStore(), assertion test checking
> "context != NULL" should be used here, instead?

Here is another comment.

+ if (parent == NULL)
+ nulls[2] = true;
+ else
+ /*
+ * We labeled dynahash contexts with just the hash table name.
+ * To make it possible to identify its parent, we also display
+ * parent's ident here.
+ */
+ if (parent->ident && strcmp(parent->name, "dynahash") == 0)
+ values[2] = CStringGetTextDatum(parent->ident);
+ else
+ values[2] = CStringGetTextDatum(parent->name);

PutMemoryContextsStatsTupleStore() doesn't need "parent" memory context,
but uses only the name of "parent" memory context. So isn't it better to use
"const char *parent" instead of "MemoryContext parent", as the argument of
the function? If we do that, we can simplify the above code.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION