Re: Re: Server is not getting started with log level as debug5 on master after commit 3147ac

On Sun, Nov 24, 2013 at 4:36 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>> On Fri, Nov 22, 2013 at 9:30 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> Again looking at it, I think better fix would be to restore 'errno'
>> from 'edata->saved_errno' in errfinish() incase the control returns
>> back to caller (elevel <= WARNING). It seems to me that this fix is
>> required anyway because if we return from errfinish (ereport/elog) to
>> caller, it should restore back 'errno', as caller might need to take
>> some action based on errno.
>> Patch to restore 'errno' in errfinish() is attached with this mail.
>
> I think this is pretty misguided. In general, there should be no
> expectation that a function call doesn't stomp on errno unless it's
> specifically documented not to --- which surely ereport() never has
> been. So generally it's the responsibility of any code that needs
> errno to be preserved to do so itself. In particular, in the case
> at hand:
>
>
> So basically, _dosmaperr() is broken and always has been, and it's
> surprising we've not seen evidence of that before. It needs to not
> try to set the real errno variable till it's done logging about it.
>
> Normally I avoid touching Windows-specific code for lack of ability
> to test it, but in this case the needed fix seems pretty clear, so
> I'll go make it.

Thanks, I have verified that the problem reported above is fixed.

I think that still this kind of problems can be there at other
places in code. I checked few places and suspecting secure_read() can
also have similar problem:

secure_read()
{
..
errno = 0;
n = SSL_read(port->ssl, ptr, len);
err = SSL_get_error(port->ssl, n);
switch (err)
{
..
..

case SSL_ERROR_SSL:
ereport(COMMERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("SSL error: %s", SSLerrmessage())));
/* fall through */
..
}

In case SSL_read sets errno, ereport will reset it and caller of
secure_read() like pq_getbyte_if_available() who perform actions based
on errno, can lead to some problem.

pq_getbyte_if_available(unsigned char *c)
{
..

r = secure_read(MyProcPort, c, 1);
if (r < 0)
{
if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR)
r = 0;
..
}

In general it is responsibility of caller to take care of errno
handling, but I am not sure it is taken care well at all places in
code and the chances of such problems were less earlier because there
was less chance that ereport would reset errno, but now it will
definitely do so.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com