On Mon, Oct 01, 2007 at 03:16:13PM +0200, Magnus Hagander wrote:
> On Mon, Oct 01, 2007 at 02:37:44PM +0200, Magnus Hagander wrote:
> > On Sat, Sep 29, 2007 at 09:01:16PM +0100, Dave Page wrote:
> > > Tom Lane wrote:
> > > > "Dave Page" <dpage@postgresql.org> writes:
> > > >>> From: Tom Lane <tgl@sss.pgh.pa.us>
> > > >>> ... It's not entirely clear whether BIO_new_fp() would avoid the
> > > >>> problematic calls, but it doesn't look like it'd be hard to try.
> > > >
> > > >> The last version of the patch I posted uses BIO_new_file() in all cases, and (from memory) BIO_get_fp() in the
non-win32case to get a FILE* to pass to fstat.
> > > >
> > > > Did you manage to get rid of the bogus-error-message problem that
> > > > afflicted the first version of the patch? If so, this way is fine.
> > >
> > > No, thats still an issue.
> >
> > A guess on this - probably the BIO stuff overwrites some internal OpenSSL
> > "errno" value, causing the wrong error to be passed up. Most likely, it's
> > not save to call BIO functions from inside the callback. My bet is that
> > it'll actually break without this patch, if you stick something that's
> > invalid in there. It's just taht we picked up the "does not exist" error
> > without calling BIO functions.
> >
> > A quick peek at the OpenSSL sources seems to confirm this.
> >
> > I think we want to either attempt to load the client certificate before we
> > connect (and before it's requested) and just queue up the error to show it
> > in only if it's requested, or we want to try some magic around
> > ERR_set_mark()/ERR_pop_to_mark() to clear out any BIO errors before we hand
> > control back.
> >
> > I'll see if I can put together a poc patch - need to reproduce the problem
> > first :-)
Yes, that was the problem. Attached patch fixes the problem for me on
Windows and Linux using the error mark functionality. It seems a lot
cleaner than the other option.
Dave - can you test this one? Assuming that works, I'll go ahead and apply
it.
I also think we have the same problem in the current code, just not for
fopen(). We trap fopen() without anything happening inside OpenSSL, but if
we get an error in the OpenSSL functions we call, something similar would
likely happen.
Should we backpatch all the stuff, or just the error push/pop thing?
//Magnus