From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: torikoshia <torikoshia@oss.nttdata.com>
To: PostgreSQL-development <pgsql-hackers@postgresql.org>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Michael Paquier <michael@paquier.xyz>
Subject: Re: Creating a function for exposing memory usage of backend process
Date: Mon, 17 Aug 2020 21:19:53 +0900
Message-ID: <083781e2-339e-9844-a02c-53a0ee3beb82@oss.nttdata.com> (raw)
In-Reply-To: <98e53274-253f-6bd1-b542-489ecbda8753@oss.nttdata.com>
References: <805313b5f7dc2e6c19b209ff930a7a61@oss.nttdata.com>
<a9d68488-8343-5d70-4612-311d597f9962@oss.nttdata.com>
<cd0e961fd42e5708fdea70f7420bf214@oss.nttdata.com>
<9ee242b1-9e70-18f5-0033-6a086e71164d@oss.nttdata.com>
<3548926512ddfaedfb5eaf967bf2f196@oss.nttdata.com>
<20200708170348.rsqry4b4k5mrsu7y@alap3.anarazel.de>
<24ea05d7bbdb652feef50caee6fcfbf8@oss.nttdata.com>
<CAP0=ZVJ2JPS41CffcG-E2CnBkfOjFRa4YoKB=A8vbWGkjEtA1w@mail.gmail.com>
<e1b81c88dc750ecd2ef03924af751567@oss.nttdata.com>
<CA+TgmoZU6Tr2mEY2Cjf5-Cce5NF6B0eknCge1v7Uk3D5H2njOA@mail.gmail.com>
<20200808014409.GA1600@paquier.xyz>
<66c9d45297ad820cd092b125aa91ce2c@oss.nttdata.com>
<98e53274-253f-6bd1-b542-489ecbda8753@oss.nttdata.com>
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@oss.nttdata.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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: masao.fujii@oss.nttdata.com, torikoshia@oss.nttdata.com, robertmhaas@gmail.com, kasahara.tatsuhito@gmail.com, andres@anarazel.de, michael@paquier.xyz
Subject: Re: Creating a function for exposing memory usage of backend process
In-Reply-To: <083781e2-339e-9844-a02c-53a0ee3beb82@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox