Обсуждение: Requesting external_pid_file with postgres -C when not initialized lead to coredump

alain radix wrote:
> Hi,
> 
> I faced a coredump when reading the value of the parameter
> "external_pid_file" when it was not initialized in postgresql.conf
> This came from the value not being specified to be initialized to en empty
> string in guc.c in the ConfigureNamesString array.
> the behavior can easily been tested with the following commands :
> initdb test
> postgres -D test -C external_pid_file
> 
> I faced the problem with version 9.3, 9.5 and 9.6 beta 1
> This seems to come from a long time ago.
> 
> I wrote a patch ( with help from Stéphane Schildknecht ) to correct the
> problem with a proper initialization.

This was fixed six days ago in a slightly different manner:

commit 0b0baf26211a98a8593279fa24635bc4dd9ac99d
Author:     Tom Lane <tgl@sss.pgh.pa.us>
AuthorDate: Thu Jun 16 12:17:03 2016 -0400
CommitDate: Thu Jun 16 12:17:38 2016 -0400
   Avoid crash in "postgres -C guc" for a GUC with a null string value.      Emit "(null)" instead, which was the
behaviorall along on platforms   that don't crash, eg OS X.  Per report from Jehan-Guillaume de Rorthais.   Back-patch
to9.2 where -C option was introduced.      Michael Paquier      Report: <20160615204036.2d35d86a@firost>
 


> The patch also removed a useless initialization of cluster_name to save a
> little memory.

Yeah, we could do away with that I suppose.


> So, here is my first patch for PostgreSQL.

Looking forward to further ones,

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jun 22, 2016 at 2:02 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> alain radix wrote:
>> So, here is my first patch for PostgreSQL.
>
> Looking forward to further ones,

A comment on this patch: this is an incorrect approach anyway.
PostmasterMain relies on this value being NULL to decide if this PID
file should be written or not, so this patch applied as-is would
result in a message outputted to stderr if the logic is not changed
there.
-- 
Michael

alain radix <alain.radix@gmail.com> writes:
> Here is a new patch with postmaster.c modification so that it check about a
> empty string instead of a null pointer.

Why should we adopt this, when there is already a better (more complete)
fix in git?

I'm not sold on the wisdom of modifying the semantics of
external_pid_file, which is what this patch effectively does.  I don't
know if anything outside our core code looks at that variable, but if
anything does, this would create issues for it.

I'm even less sold on the wisdom of changing every other GUC that
has a null bootstrap default, which is what we would also have to do
in order to avoid the identical misbehavior for other -C cases.

> The meaning is no more to avoid a core dump as you've done a change for
> that but to have the same result with postgres -C as with a request to
> pg_settings, an empty string when the parameter is not set.

Well, maybe we should make -C print an empty string not "(nil)" for
such cases.  You're right that we don't make a distinction between
NULL and "" in other code paths that display a string GUC, so doing
so here is a bit inconsistent.
        regards, tom lane

alain radix <alain.radix@gmail.com> writes:
> Another effect of the patch is that it become possible to start a cluster
> with external_pid_file='' in postgresql.conf
> It would have that same behavior as many other parameters.

I'd still be inclined to do that with something along the lines of

-    if (external_pid_file)
+    if (external_pid_file && external_pid_file[0])

rather than changing the default per se.
        regards, tom lane