Thread: initdb SegFault
Hi,
While checking initdb code, I found one segmentation fault, stack
trace for the same is:
Core was generated by `./initdb -D data6'.
Program terminated with signal 11, Segmentation fault.
#0 0x000000000040ea22 in main (argc=3, argv=0x7ffc82237308) at initdb.c:3340
3340 printf(_("\nSuccess. You can now start the database server
using:\n\n"
Analysis for the same is given below:
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.
Let me know your thoughts for the same. Similar issue exists in few
other places, if changes are ok, I can check and fix the issue in
other places also.
Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
Attachment
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