Thread: Requesting external_pid_file with postgres -C when not initialized lead to coredump

Requesting external_pid_file with postgres -C when not initialized lead to coredump

From

alain radix

Date:

21 June 2016, 18:54:29

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.

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

So, here is my first patch for PostgreSQL.

Best regards.

Alain Radix

Attachment

external_pid_file.patch

Re: Requesting external_pid_file with postgres -C when not initialized lead to coredump

From

Alvaro Herrera

Date:

21 June 2016, 20:02:38

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

Re: Requesting external_pid_file with postgres -C when not initialized lead to coredump

From

Michael Paquier

Date:

22 June 2016, 00:45:41

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

Re: Requesting external_pid_file with postgres -C when not initialized lead to coredump

From

alain radix

Date:

22 June 2016, 18:15:52

Yes Michael,

You're right, I aw this yesterday but couldn't make a patch.

Here is a new patch with postmaster.c modification so that it check about a empty string instead of a null pointer.

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.

It seems a lot better to me that different ways to request the same parameter should return the same answer.

Actually, setting from a request to pg_settings an empty string in postgresql.conf for external_pid_file would lead to an error for the postmaster.

To values of parameter should be the same reported in pg_settings and postgres -C and should be a valid settings in postgresql.conf

So, that if you create a postgresql.conf from pg_settings or postresql -C,, it would would be a valid one.

So, the patch make an empty string valid in all places and shouldn't cause problem with existing installations.

Regards.

Alain Radix

On 21 June 2016 at 23:45, Michael Paquier <michael.paquier@gmail.com> wrote:

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

Attachment

external_pid_file.patch

Re: Requesting external_pid_file with postgres -C when not initialized lead to coredump

From

Tom Lane

Date:

22 June 2016, 18:25:04

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

Re: Requesting external_pid_file with postgres -C when not initialized lead to coredump

From

alain radix

Date:

22 June 2016, 19:12:15

Hi Tom,

The actual fix do the job for preventing coredump.

Effectively, reporting "" instead of (null) would be more consistent with the values found in pg_settings.

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 started a 9.6 beta with the following lines in postgresql.conf and the only one that reported a problem is external_pid_file

external_pid_file = ''                  # write an extra PID file
unix_socket_group = ''                  # (change requires restart)
bonjour_name = ''                       # defaults to the computer name
ssl_ca_file = ''                        # (change requires restart)
ssl_crl_file = ''                       # (change requires restart)
krb_server_keyfile = ''
shared_preload_libraries = ''           # (change requires restart)
synchronous_standby_names = '' # standby servers that provide sync rep
cluster_name = ''                       # added to process titles if nonempty
default_tablespace = ''         # a tablespace name, '' uses the default
temp_tablespaces = ''                   # a list of tablespace names, '' uses
local_preload_libraries = ''
session_preload_libraries = ''

It seems to me that the general behavior it to consider an empty string as an unset parameter in postgresql.conf, why should external_pid_file be an exception ?

I would prefer postgresql -C to report an empty string to be consistent with pg_settings in a backport of the fix.

I also would prefer external_pid_file to behave as other parameters in the next versions.

Regards.

Alain

On 22 June 2016 at 17:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

Re: Requesting external_pid_file with postgres -C when not initialized lead to coredump

From

Tom Lane

Date:

22 June 2016, 19:51:48

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

Re: Requesting external_pid_file with postgres -C when not initialized lead to coredump

From

alain radix

Date:

23 June 2016, 13:22:39

Testing that external_pid_file was not null didn't seem necessary to me because it's initialized to '' and background workers could only start after the startup piece of code.

If external_pid_file is set to null while the server run, then I prefer to not hide the problem as it would only be reported at the server shutdown

I tested with ALTER SYSTEM SET but couldn't set external_pid_file to null, only to ''

This another good reason to use '' instead of null, because it can be set with ALTER SYSTEM SET even if I see no reason to ;-)

Using strlen(external_pid_file) instead of external_pid_file[0] seems more readable to me.

I agree that it cost a few cpu cycles more.

I'm also unsure that testing if(A && B) would always be compiled to A being tested before B so that it won't always protect from testing external_pid_file[0] with external_pid_file being null.

To ensure that, I would prefer :

if(! external_pid_file){
if(external_pid_file[0])

But, I see it at useless protective code against an impossible situation.

Maybe I'm wrong, but I don't see how it could happen.

About parameters being null, as they're reported as '' in pg_settings and ALTER SYSTEM SET can only set them to '', I consider this should be avoided.

I used all parameters reported to '' in pg_settings after initdb and found only external_pid_file to crash postgres -C

Alain

PS : Thanks for pgBackRest ;-)

On 22 June 2016 at 18:51, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

Alain Radix