Re: [BUGS] libpq does not manage SSL callbacks properly when other libraries are involved.

Hi,

After experiencing a seg fault on RHEL5's command line php, I did the
following investigation.

As I dot not have a RHEL5 box available with debugging tools, I
reproduced the bug on CentOS5.

Reproduction on Centos5
-----------------------
[root(at)unknown-00-16-3e-30-f0-0d ~]# php -f test-pgsql.php
PHP Fatal error: Uncaught exception 'PDOException' with message
'SQLSTATE[08006] [7] FATAL: no pg_hba.conf entry for host
"59.167.146.4", user "nouser", database "fsd", SSL off' in
/root/test-pgsql.php:3
Stack trace:
#0 /root/test-pgsql.php(3): PDO->__construct('pgsql:host=thoe...',
'nouser', 'nopass')
#1 {main}
thrown in /root/test-pgsql.php on line 3
Segmentation fault

[root(at)unknown-00-16-3e-30-f0-0d ~]# cat test-pgsql.php
<?php

$x = new
PDO("pgsql:host=connectableserver;port=5437;dbname=fsd","nouser","nopass");

?>
[root(at)unknown-00-16-3e-30-f0-0d ~]# php -f test-pgsql.php
PHP Fatal error: Uncaught exception 'PDOException' with message
'SQLSTATE[08006] [7] FATAL: no pg_hba.conf entry for host
"59.167.146.4", user "nouser", database "fsd", SSL off' in
/root/test-pgsql.php:3
Stack trace:
#0 /root/test-pgsql.php(3): PDO->__construct('pgsql:host=thoe...',
'nouser', 'nopass')
#1 {main}
thrown in /root/test-pgsql.php on line 3
Segmentation fault
[root(at)unknown-00-16-3e-30-f0-0d ~]# gdb php
GNU gdb Red Hat Linux (6.5-25.el5_1.1rh)
Copyright (C) 2006 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain
conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB. Type "show warranty" for details.
This GDB was configured as "i386-redhat-linux-gnu"...(no debugging
symbols found)
Using host libthread_db library "/lib/libthread_db.so.1".

(gdb) run -f test-pgsql.php
Starting program: /usr/bin/php -f test-pgsql.php
(no debugging symbols found)
(no debugging symbols found)
(no debugging symbols found)
(no debugging symbols found)
(no debugging symbols found)
[Thread debugging using libthread_db enabled]
[New Thread -1208912160 (LWP 27037)]
PHP Fatal error: Uncaught exception 'PDOException' with message
'SQLSTATE[08006] [7] FATAL: no pg_hba.conf entry for host
"59.167.146.4", user "nouser", database "fsd", SSL off' in
/root/test-pgsql.php:3
Stack trace:
#0 /root/test-pgsql.php(3): PDO->__construct('pgsql:host=thoe...',
'nouser', 'nopass')
#1 {main}
thrown in /root/test-pgsql.php on line 3

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1208912160 (LWP 27037)]
0x003248b0 in ?? ()
(gdb) bt
#0 0x003248b0 in ?? ()
#1 0x003996c5 in CRYPTO_lock () from /lib/libcrypto.so.6
#2 0x003efdbd in ERR_get_implementation () from /lib/libcrypto.so.6
#3 0x003efb90 in ERR_free_strings () from /lib/libcrypto.so.6
#4 0x00588107 in Curl_ossl_cleanup () from /usr/lib/libcurl.so.3
#5 0x00599370 in Curl_ssl_cleanup () from /usr/lib/libcurl.so.3
#6 0x005910c3 in curl_global_cleanup () from /usr/lib/libcurl.so.3
#7 0x0808d85b in zm_shutdown_curl ()
#8 0x081cecbe in module_destructor ()
#9 0x081d42a7 in zend_hash_quick_find ()
#10 0x081d4538 in zend_hash_graceful_reverse_destroy ()
#11 0x081cb89e in zend_shutdown ()
#12 0x0818e0ff in php_module_shutdown ()
#13 0x0824168b in main ()

After googling around a bit, I found these relevant bug links;

http://bugs.php.net/bug.php?id=40926
https://bugs.launchpad.net/ubuntu/+source/php5/+bug/63141
http://bugs.debian.org/411982 <http://bugs.debian.org/411982>

Following up the php bug report appeared to give the most useful outcome;

This is part of a comment from the php bug comment history;

*[12 Nov 2007 2:45pm UTC] sam at zoy dot org*

Hello, I did read the sources and studied them, and I can confirm
that it is a matter of callback jumping to an invalid address.

libpq's init_ssl_system() installs callbacks by calling
CRYPTO_set_id_callback() and CRYPTO_set_locking_callback(). This
function is called each time initialize_SSL() is called (for instance
through the PHP pg_connect() function) and does not keep a reference
counter, so libpq's destroy_SSL() has no way to know that it should
call a destroy_ssl_system() function, and there is no such function
anyway. So the callbacks are never removed.

But then, upon cleanup, PHP calls zend_shutdown() which properly
unloads pgsql.so and therefore the unused libpq.

Finally, the zend_shutdown procedure calls zm_shutdown_curl()
which in turn calls curl_global_cleanup() which leads to an
ERR_free_strings() call and eventually a CRYPTO_lock() call.
CRYPTO_lock() checks whether there are any callbacks to call,
finds one (the one installed by libpg), calls it, and crashes
because libpq was unloaded and hence the callback is no longer
in mapped memory.

After noting that it is SSL related, I adjusted my test script to show
the following (added sslmode=disable);

[root(at)unknown-00-16-3e-30-f0-0d ~]# cat test-pgsql.php

<?php

$x = new PDO("pgsql:host=connectablehost;port=5437;dbname=fsd;sslmode=disable","nouser","nopass");

?>

[root(at)unknown-00-16-3e-30-f0-0d ~]# php -f test-pgsql.php

PHP Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[08006] [7] FATAL: no pg_hba.conf entry for host "xx.xx.xx.xx", user "nouser", database "fsd", SSL off' in /root/test-pgsql.php:3

Stack trace:

#0 /root/test-pgsql.php(3): PDO->__construct('pgsql:host=thoe...', 'nouser', 'nopass')

#1 {main}

thrown in /root/test-pgsql.php on line 3

As a result, the crash has gone away. Are the comments in the PHP
comment accurate and is it reasonable to count calls to SSL in the way
suggested? As currently the callback remains even if libpq is unloaded
from memory, which is what's causing this problem. The callback should
be unregistered when we close our own SSL stuff?

Is it possible to get this fixed and possibly backported?

Thanks

Russell Smith

Russell Smith wrote:
> Hi,
>
> After experiencing a seg fault on RHEL5's command line php, I did the
> following investigation.
>
> As I dot not have a RHEL5 box available with debugging tools, I
> reproduced the bug on CentOS5.
Hi,

I've not received any feedback on this bug in a week, is anybody looking
at it. Is there anything I'm doing wrong with my report of this bug?

Thanks

Russell.

***PUSH***

this bug is really some annoyance if you use automatic build environments.
I'm using phpunit to run tests and as soon as postgres is involved the php
cli environment segfaults at the end. this can be worked around by disabling
ssl but it would be great if the underlying bug got fixed.
--
View this message in context: http://www.nabble.com/libpq-does-not-manage-SSL-callbacks-properly-when-other-libraries-are-involved.-tp18108184p19212172.html
Sent from the PostgreSQL - bugs mailing list archive at Nabble.com.

PoolSnoopy wrote:
>
> ***PUSH***
>
> this bug is really some annoyance if you use automatic build environments.
> I'm using phpunit to run tests and as soon as postgres is involved the php
> cli environment segfaults at the end. this can be worked around by disabling
> ssl but it would be great if the underlying bug got fixed.

This is PHP's bug, isn't it? Why are you complaining here?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> PoolSnoopy wrote:
>
>> ***PUSH***
>>
>> this bug is really some annoyance if you use automatic build environments.
>> I'm using phpunit to run tests and as soon as postgres is involved the php
>> cli environment segfaults at the end. this can be worked around by disabling
>> ssl but it would be great if the underlying bug got fixed.
>>
>
> This is PHP's bug, isn't it? Why are you complaining here
No, this is a problem with the callback/exit functions used by
PostgreSQL. We setup callback functions when we use SSL, if somebody
else uses SSL we can create a problem.

I thought my original report was detailed enough to explain where the
problem is coming from. Excerpt from original report;

This is part of a comment from the php bug comment history;

*[12 Nov 2007 2:45pm UTC] sam at zoy dot org*

Hello, I did read the sources and studied them, and I can confirm
that it is a matter of callback jumping to an invalid address.

libpq's init_ssl_system() installs callbacks by calling
CRYPTO_set_id_callback() and CRYPTO_set_locking_callback(). This
function is called each time initialize_SSL() is called (for instance
through the PHP pg_connect() function) and does not keep a reference
counter, so libpq's destroy_SSL() has no way to know that it should
call a destroy_ssl_system() function, and there is no such function
anyway. So the callbacks are never removed.

But then, upon cleanup, PHP calls zend_shutdown() which properly
unloads pgsql.so and therefore the unused libpq.

Finally, the zend_shutdown procedure calls zm_shutdown_curl()
which in turn calls curl_global_cleanup() which leads to an
ERR_free_strings() call and eventually a CRYPTO_lock() call.
CRYPTO_lock() checks whether there are any callbacks to call,
finds one (the one installed by libpg), calls it, and crashes
because libpq was unloaded and hence the callback is no longer
in mapped memory.

--

Basically postgresql doesn't cancel the callbacks to itself when the pg
connection is shut down. So if the libpq library is unloaded before
other libraries that use SSL you get a crash as described above. PHP
has suggested the fix is to keep a reference counter in libpq so knows
when to remove the callbacks.

This is a complicated bug, but without real evidence there is no way to
go to back to PHP and say it's their fault. Their analysis is
relatively comprehensive compared to the feedback that's been posted
here so far. I'm not sure how best to setup an environment to replicate
the bug in a way I can debug it. And even if I get to the point of
nailing it down, I'll just be back asking questions about how you would
fix it because I know very little about SSL.

All that said, a quick poke in the source of PostgreSQL says that
fe-secure.c sets callbacks using CRYPTO_set_xx_callback(...). These are
only set in the threaded version it appears. Which is pretty much
default in all the installations I encounter.

My google research indicated we need to call
CRYPTO_set_xx_callback(NULL) when we exit. but that's not done. One
idea for a fix is to add a counter to the initialize_ssl function and
when destory_ssl is called, decrement the counter. If it reaches 0 then
call CRYPT_set_xx_callback(NULL) to remove the callbacks. This is a
windows SSL thread that crashes iexplore and testifies to the same
problem http://www.mail-archive.com/openssl-users(at)openssl(dot)org/msg53869.html

Thoughts?

Regards

Russell Smith

Russell Smith wrote:
> Alvaro Herrera wrote:
> > PoolSnoopy wrote:
> >
> >> this bug is really some annoyance if you use automatic build environments.
> >> I'm using phpunit to run tests and as soon as postgres is involved the php
> >> cli environment segfaults at the end. this can be worked around by disabling
> >> ssl but it would be great if the underlying bug got fixed.
> >>
> >
> > This is PHP's bug, isn't it? Why are you complaining here
> No, this is a problem with the callback/exit functions used by
> PostgreSQL. We setup callback functions when we use SSL, if somebody
> else uses SSL we can create a problem.

Ok, so it seems you're correct; there is more evidence to be found by
searching other projects' mailing lists, for example as a starting point
http://markmail.org/search/?q=+CRYPTO_set_locking_callback%28NULL%29

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Russell Smith wrote:
> Alvaro Herrera wrote:
> > PoolSnoopy wrote:
> >
> >> ***PUSH***
> >>
> >> this bug is really some annoyance if you use automatic build environments.
> >> I'm using phpunit to run tests and as soon as postgres is involved the php
> >> cli environment segfaults at the end. this can be worked around by disabling
> >> ssl but it would be great if the underlying bug got fixed.
> >>
> >
> > This is PHP's bug, isn't it? Why are you complaining here
> No, this is a problem with the callback/exit functions used by
> PostgreSQL. We setup callback functions when we use SSL, if somebody
> else uses SSL we can create a problem.
>
> I thought my original report was detailed enough to explain where the
> problem is coming from. Excerpt from original report;
>
> This is part of a comment from the php bug comment history;
>
> *[12 Nov 2007 2:45pm UTC] sam at zoy dot org*
>
> Hello, I did read the sources and studied them, and I can confirm
> that it is a matter of callback jumping to an invalid address.
>
> libpq's init_ssl_system() installs callbacks by calling
> CRYPTO_set_id_callback() and CRYPTO_set_locking_callback(). This
> function is called each time initialize_SSL() is called (for instance
> through the PHP pg_connect() function) and does not keep a reference
> counter, so libpq's destroy_SSL() has no way to know that it should
> call a destroy_ssl_system() function, and there is no such function
> anyway. So the callbacks are never removed.
>
> But then, upon cleanup, PHP calls zend_shutdown() which properly
> unloads pgsql.so and therefore the unused libpq.
>
> Finally, the zend_shutdown procedure calls zm_shutdown_curl()
> which in turn calls curl_global_cleanup() which leads to an
> ERR_free_strings() call and eventually a CRYPTO_lock() call.
> CRYPTO_lock() checks whether there are any callbacks to call,
> finds one (the one installed by libpg), calls it, and crashes
> because libpq was unloaded and hence the callback is no longer
> in mapped memory.
>
> --
>
> Basically postgresql doesn't cancel the callbacks to itself when the pg
> connection is shut down. So if the libpq library is unloaded before
> other libraries that use SSL you get a crash as described above. PHP
> has suggested the fix is to keep a reference counter in libpq so knows
> when to remove the callbacks.
>
> This is a complicated bug, but without real evidence there is no way to
> go to back to PHP and say it's their fault. Their analysis is
> relatively comprehensive compared to the feedback that's been posted
> here so far. I'm not sure how best to setup an environment to replicate
> the bug in a way I can debug it. And even if I get to the point of
> nailing it down, I'll just be back asking questions about how you would
> fix it because I know very little about SSL.
>
> All that said, a quick poke in the source of PostgreSQL says that
> fe-secure.c sets callbacks using CRYPTO_set_xx_callback(...). These are
> only set in the threaded version it appears. Which is pretty much
> default in all the installations I encounter.
>
> My google research indicated we need to call
> CRYPTO_set_xx_callback(NULL) when we exit. but that's not done. One
> idea for a fix is to add a counter to the initialize_ssl function and
> when destory_ssl is called, decrement the counter. If it reaches 0 then
> call CRYPT_set_xx_callback(NULL) to remove the callbacks. This is a
> windows SSL thread that crashes iexplore and testifies to the same
> problem http://www.mail-archive.com/openssl-users(at)openssl(dot)org/msg53869.html

Sorry for the delay in addressing this bug report.

Your analysis of this problem is right on target. When the SSL
callbacks were implemented for threaded libpq, there was never any
thought on the effect of unloading libpq while the callbacks were still
registered.

The attached patch unregisters the callback on the close of the last
libpq connection. Fortunately we require PQfinish() even if the
connection request failed, meaning there should be proper accounting of
the number of open connections with the method used in this patch.

We do leak some memory for every load/unload of libpq, but the leaks
extend beyond the SSL code to the rest of libpq so I didn't attempt to
address that in this patch (and no one has complained about it).

I also could have implemented a function to unload the SSL callbacks.
It would have to have been called before libpq was unloaded, but I
considered it inconvenient and unlikely to be adopted by applications
using libpq in the short-term.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Russell Smith wrote:
>> Alvaro Herrera wrote:
>>> PoolSnoopy wrote:
>>>
>>>> ***PUSH***
>>>>
>>>> this bug is really some annoyance if you use automatic build environments.
>>>> I'm using phpunit to run tests and as soon as postgres is involved the php
>>>> cli environment segfaults at the end. this can be worked around by disabling
>>>> ssl but it would be great if the underlying bug got fixed.
>>>>
>>> This is PHP's bug, isn't it? Why are you complaining here
>> No, this is a problem with the callback/exit functions used by
>> PostgreSQL. We setup callback functions when we use SSL, if somebody
>> else uses SSL we can create a problem.
>>
>> I thought my original report was detailed enough to explain where the
>> problem is coming from. Excerpt from original report;
>>
>> This is part of a comment from the php bug comment history;
>>
>> *[12 Nov 2007 2:45pm UTC] sam at zoy dot org*
>>
>> Hello, I did read the sources and studied them, and I can confirm
>> that it is a matter of callback jumping to an invalid address.
>>
>> libpq's init_ssl_system() installs callbacks by calling
>> CRYPTO_set_id_callback() and CRYPTO_set_locking_callback(). This
>> function is called each time initialize_SSL() is called (for instance
>> through the PHP pg_connect() function) and does not keep a reference
>> counter, so libpq's destroy_SSL() has no way to know that it should
>> call a destroy_ssl_system() function, and there is no such function
>> anyway. So the callbacks are never removed.
>>
>> But then, upon cleanup, PHP calls zend_shutdown() which properly
>> unloads pgsql.so and therefore the unused libpq.
>>
>> Finally, the zend_shutdown procedure calls zm_shutdown_curl()
>> which in turn calls curl_global_cleanup() which leads to an
>> ERR_free_strings() call and eventually a CRYPTO_lock() call.
>> CRYPTO_lock() checks whether there are any callbacks to call,
>> finds one (the one installed by libpg), calls it, and crashes
>> because libpq was unloaded and hence the callback is no longer
>> in mapped memory.
>>
>> --
>>
>> Basically postgresql doesn't cancel the callbacks to itself when the pg
>> connection is shut down. So if the libpq library is unloaded before
>> other libraries that use SSL you get a crash as described above. PHP
>> has suggested the fix is to keep a reference counter in libpq so knows
>> when to remove the callbacks.
>>
>> This is a complicated bug, but without real evidence there is no way to
>> go to back to PHP and say it's their fault. Their analysis is
>> relatively comprehensive compared to the feedback that's been posted
>> here so far. I'm not sure how best to setup an environment to replicate
>> the bug in a way I can debug it. And even if I get to the point of
>> nailing it down, I'll just be back asking questions about how you would
>> fix it because I know very little about SSL.
>>
>> All that said, a quick poke in the source of PostgreSQL says that
>> fe-secure.c sets callbacks using CRYPTO_set_xx_callback(...). These are
>> only set in the threaded version it appears. Which is pretty much
>> default in all the installations I encounter.
>>
>> My google research indicated we need to call
>> CRYPTO_set_xx_callback(NULL) when we exit. but that's not done. One
>> idea for a fix is to add a counter to the initialize_ssl function and
>> when destory_ssl is called, decrement the counter. If it reaches 0 then
>> call CRYPT_set_xx_callback(NULL) to remove the callbacks. This is a
>> windows SSL thread that crashes iexplore and testifies to the same
>> problem http://www.mail-archive.com/openssl-users(at)openssl(dot)org/msg53869.html
>
> Sorry for the delay in addressing this bug report.
>
> Your analysis of this problem is right on target. When the SSL
> callbacks were implemented for threaded libpq, there was never any
> thought on the effect of unloading libpq while the callbacks were still
> registered.
>
> The attached patch unregisters the callback on the close of the last
> libpq connection. Fortunately we require PQfinish() even if the
> connection request failed, meaning there should be proper accounting of
> the number of open connections with the method used in this patch.
>
> We do leak some memory for every load/unload of libpq, but the leaks
> extend beyond the SSL code to the rest of libpq so I didn't attempt to
> address that in this patch (and no one has complained about it).
>
> I also could have implemented a function to unload the SSL callbacks.
> It would have to have been called before libpq was unloaded, but I
> considered it inconvenient and unlikely to be adopted by applications
> using libpq in the short-term.

I don't see why destroy_ssl_system sets up it's own mutex (that's also
called init_mutex). I think it'd make more sense to make the mutex
created in init_ssl_system() visible to the destroy function, and make
use of that one instead. You'll need to somehow interlock against these
two functions running on different threads after all.

Also, the code for destroying/unlinking appears to never be called.. The
callchain ends in pqsecure_destroy(), which is never called.

//Magnus

Magnus Hagander wrote:
> > Your analysis of this problem is right on target. When the SSL
> > callbacks were implemented for threaded libpq, there was never any
> > thought on the effect of unloading libpq while the callbacks were still
> > registered.
> >
> > The attached patch unregisters the callback on the close of the last
> > libpq connection. Fortunately we require PQfinish() even if the
> > connection request failed, meaning there should be proper accounting of
> > the number of open connections with the method used in this patch.
> >
> > We do leak some memory for every load/unload of libpq, but the leaks
> > extend beyond the SSL code to the rest of libpq so I didn't attempt to
> > address that in this patch (and no one has complained about it).
> >
> > I also could have implemented a function to unload the SSL callbacks.
> > It would have to have been called before libpq was unloaded, but I
> > considered it inconvenient and unlikely to be adopted by applications
> > using libpq in the short-term.
>
> I don't see why destroy_ssl_system sets up it's own mutex (that's also
> called init_mutex). I think it'd make more sense to make the mutex
> created in init_ssl_system() visible to the destroy function, and make
> use of that one instead. You'll need to somehow interlock against these
> two functions running on different threads after all.
>
>
> Also, the code for destroying/unlinking appears to never be called.. The
> callchain ends in pqsecure_destroy(), which is never called.

Thanks for the review, Magnus. I have adjusted the patch to use the
same mutex every time the counter is accessed, and adjusted the
pqsecure_destroy() call to properly decrement in the right place.

Also, I renamed the libpq global destroy function to be clearer
(the function is not exported).

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:

> Thanks for the review, Magnus. I have adjusted the patch to use the
> same mutex every time the counter is accessed, and adjusted the
> pqsecure_destroy() call to properly decrement in the right place.
>
> Also, I renamed the libpq global destroy function to be clearer
> (the function is not exported).

There's a problem in this patch which is that it is inconsistent in its
use of the ENABLE_THREAD_SAFETY symbol. init_ssl_system() is only going
to keep the refcount in the threaded compile; but the safeguards are
needed even when threading is not enabled. Moreover,
destroy_ssl_system() is locking thread mutexes outside
ENABLE_THREAD_SAFETY which is going to cause non-threaded builds to
fail.

As a suggestion, I'd recommend not fooling around with backend files
when you're only modifying libpq. It enlarges the patch without
benefit. I think that patch should be committed separately.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> Bruce Momjian wrote:
>
> > Thanks for the review, Magnus. I have adjusted the patch to use the
> > same mutex every time the counter is accessed, and adjusted the
> > pqsecure_destroy() call to properly decrement in the right place.
> >
> > Also, I renamed the libpq global destroy function to be clearer
> > (the function is not exported).
>
> There's a problem in this patch which is that it is inconsistent in its
> use of the ENABLE_THREAD_SAFETY symbol. init_ssl_system() is only going
> to keep the refcount in the threaded compile; but the safeguards are
> needed even when threading is not enabled. Moreover,

Actually, CRYPTO_set_locking_callback() and CRYPTO_set_id_callbac() are
needed only for threaded SSL programs; I have added a comments
mentioning that.

> destroy_ssl_system() is locking thread mutexes outside
> ENABLE_THREAD_SAFETY which is going to cause non-threaded builds to
> fail.

Yes, my defines were very messed up; updated version attached.

> As a suggestion, I'd recommend not fooling around with backend files
> when you're only modifying libpq. It enlarges the patch without
> benefit. I think that patch should be committed separately.

OK, I will do that, though the backend change is being made to be
consistent with the front end.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Yes, my defines were very messed up; updated version attached.
>
Hi,

I've not done a review of this patch, however I did backport it to 8.3
(as attached in unified diff). The patch wasn't made for PG purposes, so
it's not in context diff. I tested the backported patch and the issues I
was experiencing with the initial bug report have stopped. So the fix
works for the initial reported problem.

Will this be back patched when it's committed?

Regards

Russell

Russell Smith wrote:
> Bruce Momjian wrote:
> > Yes, my defines were very messed up; updated version attached.
> >
> Hi,
>
> I've not done a review of this patch, however I did backport it to 8.3
> (as attached in unified diff). The patch wasn't made for PG purposes, so
> it's not in context diff. I tested the backported patch and the issues I
> was experiencing with the initial bug report have stopped. So the fix
> works for the initial reported problem.

Wow, that is great. I was only guessing on the cause but it seemed
logical, and your testing confirmed it.

> Will this be back patched when it's committed?

This is not something we would typically backpatch because of the danger
of introducing some unexpected change in libpq. We can provide a patch
to anyone who needs it, or if the community wants it backpatched I can
certainly do that.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Russell Smith wrote:

> > Will this be back patched when it's committed?
>
> This is not something we would typically backpatch because of the danger
> of introducing some unexpected change in libpq. We can provide a patch
> to anyone who needs it, or if the community wants it backpatched I can
> certainly do that.

It isn't? It does seem like a bug, which we do typically backpatch ...

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Bruce Momjian wrote:
>> This is not something we would typically backpatch because of the danger
>> of introducing some unexpected change in libpq. We can provide a patch
>> to anyone who needs it, or if the community wants it backpatched I can
>> certainly do that.

> It isn't? It does seem like a bug, which we do typically backpatch ...

Well, it's a risk-reward tradeoff. In this case it seems like there's
a nontrivial risk of creating new bugs against fixing a problem that
evidently affects very few people. I concur with Bruce's feeling that
we shouldn't backpatch ... at least not now. Once the patch has been
through beta testing we could reconsider.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>
>> Bruce Momjian wrote:
>>
>>> This is not something we would typically backpatch because of the danger
>>> of introducing some unexpected change in libpq. We can provide a patch
>>> to anyone who needs it, or if the community wants it backpatched I can
>>> certainly do that.
>>>

If we start deciding we are not backpatching fixes that we know cause
crashes, where is the limit?

>> It isn't? It does seem like a bug, which we do typically backpatch ...
>>
>
> Well, it's a risk-reward tradeoff. In this case it seems like there's
> a nontrivial risk of creating new bugs against fixing a problem that
> evidently affects very few people. I concur with Bruce's feeling that
> we shouldn't backpatch ... at least not now. Once the patch has been
> through beta testing we could reconsider.
>
> regards, tom lane
>
I would like to see this backpatched. Even though the PostgreSQL
community hasn't seen a lot of complaints, there have been a number of
reports where the bug has caused crashes. Ubuntu launchpad has 6
duplicates for this bug. php has a bug report for it. So it's not like
people don't know about it. They just didn't know how to fix it. All
that said, I agree it's safer to wait until the 8.4 beta cycle has given
this code change a good run before proceeding. In the mean time
distributions can either backpatch it themselves or wait for PostgreSQL
community to apply the patch.

For the environment where I have this problem, I think it's still going
to be a up hill battle to get RedHat to incorporate the fix into RHEL5.
That's whichever route the community takes with backpatching.

Russell.

Russell Smith wrote:
> Tom Lane wrote:
> > Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> >
> >> Bruce Momjian wrote:
> >>
> >>> This is not something we would typically backpatch because of the danger
> >>> of introducing some unexpected change in libpq. We can provide a patch
> >>> to anyone who needs it, or if the community wants it backpatched I can
> >>> certainly do that.
> >>>
>
> If we start deciding we are not backpatching fixes that we know cause
> crashes, where is the limit?

Stability. That is our limit (goal).

A foolish consistency is the hobgoblin of little
minds, adored by little statesmen and philosophers and divines.
Ralph Waldo Emerson

> >> It isn't? It does seem like a bug, which we do typically backpatch ...
> >>
> >
> > Well, it's a risk-reward tradeoff. In this case it seems like there's
> > a nontrivial risk of creating new bugs against fixing a problem that
> > evidently affects very few people. I concur with Bruce's feeling that
> > we shouldn't backpatch ... at least not now. Once the patch has been
> > through beta testing we could reconsider.
> >
> > regards, tom lane
> >
> I would like to see this backpatched. Even though the PostgreSQL
> community hasn't seen a lot of complaints, there have been a number of
> reports where the bug has caused crashes. Ubuntu launchpad has 6
> duplicates for this bug. php has a bug report for it. So it's not like

Wow, that is interesting.

> people don't know about it. They just didn't know how to fix it. All
> that said, I agree it's safer to wait until the 8.4 beta cycle has given
> this code change a good run before proceeding. In the mean time
> distributions can either backpatch it themselves or wait for PostgreSQL
> community to apply the patch.
>
> For the environment where I have this problem, I think it's still going
> to be a up hill battle to get RedHat to incorporate the fix into RHEL5.
> That's whichever route the community takes with backpatching.

Yea, it is a shame we didn't find/fix this earlier. It is reproducable
so I am surprised we did not hear about it sooner.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Thanks for the review, Magnus. I have adjusted the patch to use the
> same mutex every time the counter is accessed, and adjusted the
> pqsecure_destroy() call to properly decrement in the right place.
>
> Also, I renamed the libpq global destroy function to be clearer
> (the function is not exported).

Here is an updated version of the patch to match CVS HEAD.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Bruce Momjian wrote:
>> Thanks for the review, Magnus. I have adjusted the patch to use the
>> same mutex every time the counter is accessed, and adjusted the
>> pqsecure_destroy() call to properly decrement in the right place.
>>
>> Also, I renamed the libpq global destroy function to be clearer
>> (the function is not exported).
>
> Here is an updated version of the patch to match CVS HEAD.

I've updated it to match what's CVS HEAD now, and made some minor
modifications. Renamed destroySSL() to make it consistent with
initializeSSL(). Added and changed some comments. ssldiff.patch contains
my changes against Bruce's patch.

I also removed the #ifdef NOT_USED parts. They are in CVS history if we
need them, and they're trivial things anyway, so I think this is much
cleaner.

With this, it looks fine to me. Especially since we've seen some testing
from the PHP folks already.

//Magnus

Magnus Hagander wrote:
> Bruce Momjian wrote:
>> Bruce Momjian wrote:
>>> Thanks for the review, Magnus. I have adjusted the patch to use the
>>> same mutex every time the counter is accessed, and adjusted the
>>> pqsecure_destroy() call to properly decrement in the right place.
>>>
>>> Also, I renamed the libpq global destroy function to be clearer
>>> (the function is not exported).
>> Here is an updated version of the patch to match CVS HEAD.
>
> I've updated it to match what's CVS HEAD now, and made some minor
> modifications. Renamed destroySSL() to make it consistent with
> initializeSSL(). Added and changed some comments. ssldiff.patch contains
> my changes against Bruce's patch.

I've applied this version for Bruce.

//Magnus