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

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?

Regards,

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