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 <torikos...@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
