initdb SegFault

vignesh C <vignesh21@gmail.com> writes:

createPQExpBuffer allocates memory and returns the pointer, there is a
possibility that createPQExpBuffer can return NULL pointer in case of
malloc failiure, but initdb's main function does not check this
condition. During malloc failure when pointer is accessed it results
in segmentation fault. Made changes to check and exit if
createPQExpBuffer return's NULL pointer. Patch for the same is
attached.

I can't get excited about this, for several reasons.

1) The probability of it happening in the field is not
distinguishable from zero, surely. I imagine you forced this
failure by making a debugging malloc fail occasionally.

2) If we really are out of memory at this point, we'd have just as good
odds that some allocation request inside pg_log_error() would fail.
There's no practical way to ensure that that code path remains free
of malloc attempts. (Not to mention cleanup_directories_atexit().)

3) In the end, an initdb failure is an initdb failure. This change
doesn't improve robustness by any useful metric, it just adds an
untestable code path. If we could recover somehow, it'd be more
interesting to spend time on.

BTW, looking at the small minority of places that bother to test
for createPQExpBuffer failure, the correct test for that seems
to be PQExpBufferBroken().

regards, tom lane

Hi,

On 2019-11-19 10:16:02 -0500, Tom Lane wrote:

vignesh C <vignesh21@gmail.com> writes:

createPQExpBuffer allocates memory and returns the pointer, there is a
possibility that createPQExpBuffer can return NULL pointer in case of
malloc failiure, but initdb's main function does not check this
condition. During malloc failure when pointer is accessed it results
in segmentation fault. Made changes to check and exit if
createPQExpBuffer return's NULL pointer. Patch for the same is
attached.

I can't get excited about this, for several reasons.

1) The probability of it happening in the field is not
distinguishable from zero, surely. I imagine you forced this
failure by making a debugging malloc fail occasionally.

Agreed wrt this specific failure scenario. It does however seem not
great that callsites for PQExpBuffer ought to check every call for
allocation failures, in the general case.

I do think it'd be reasonable to move the cases where "graceful" dealing
with OOM isn't necessary ought to really use an interface that
internally errors out on memory allocation failures. Kinda thinking we
ought to slowly move such paths towards stringinfo...

2) If we really are out of memory at this point, we'd have just as good
odds that some allocation request inside pg_log_error() would fail.
There's no practical way to ensure that that code path remains free
of malloc attempts. (Not to mention cleanup_directories_atexit().)

I wonder if, for frontend paths, a simplified error handling path would
be worthwhile for OOM paths. Doing only a write() or such to print an
error message.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

Agreed wrt this specific failure scenario. It does however seem not
great that callsites for PQExpBuffer ought to check every call for
allocation failures, in the general case.

It is possible to check just once at the end, using the PQExpBufferBroken
API, and I believe that libpq for instance is fairly careful about that.

I agree that programs that just need to print something and exit could
perhaps ask pqexpbuffer.c to handle that for them. (But initdb still
doesn't fall in that category, because of its very nontrivial atexit
handler :-(.)

I wonder if, for frontend paths, a simplified error handling path would
be worthwhile for OOM paths. Doing only a write() or such to print an
error message.

Perhaps. You wouldn't get any translation --- but then, gettext is
probably going to fail anyway under such conditions.

regards, tom lane

At Tue, 19 Nov 2019 12:06:50 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in

Andres Freund <andres@anarazel.de> writes:

Agreed wrt this specific failure scenario. It does however seem not
great that callsites for PQExpBuffer ought to check every call for
allocation failures, in the general case.

It is possible to check just once at the end, using the PQExpBufferBroken
API, and I believe that libpq for instance is fairly careful about that.

FWIW, I looked though the callers of PQExpBuffer.

pqGetErrorNotice3 seems ingoring OOM on message buffer when !isError,
then sets NULL to res->errMsg. getParameterStatus doesn't check that
before use, too.

Most of the callers of PQExpBufferDataBroken use libpq_gettext("out of
memory"). And some of them do strdup(libpq_gettext()).

Not restricting to libpq functions,

dblink_connstr_check complains as "password is required" when
PQconninfoParse hits OOM.

libpqrcv_check_conninfo() will show '(null)' or maybe get SEGV on some
platforms when PQconninfoParse() hits OOM, since it uses err without
null checking. pg_basebackup, pg_dumpall and pg_isready is doing the
same thing.

I agree that programs that just need to print something and exit could
perhaps ask pqexpbuffer.c to handle that for them. (But initdb still
doesn't fall in that category, because of its very nontrivial atexit
handler :-(.)

I wonder if, for frontend paths, a simplified error handling path would
be worthwhile for OOM paths. Doing only a write() or such to print an
error message.

Perhaps. You wouldn't get any translation --- but then, gettext is
probably going to fail anyway under such conditions.

I think we should refrain from translating in the cases.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center