There's also a test failure on travis which is likely related to the patch:

TEST 5293/12359 [ext/openssl/tests/bug55646.phpt]
========DIFF========
001+ Segmentation fault
001- A: IT 互
002- B: 49 54 20 e4 ba 92
003- C: IT 互
004- D: 49 54 20 e4 ba 92
========DONE========
FAIL Bug #55646: textual input in openssl_csr_new() is not expected in UTF-8 [ext/openssl/tests/bug55646.phpt]

@nikic
I will check the test failure, destroy on shutdown and make this queue per thread.

I see the bug has a reproduction script, could you add it as a test?

Hi @smalyshev, i add test scripts on the last commit

@nikic
On the lasts commits the error queue are destroyed on request shutdown and module shutdown.
Also the problems reported by travis are fixed.
Now i will work on make this queue per-thread global.

@nikic
Check last commit, now the error queue are per-thread global.

@nikic I think this fix is complete.

@wfelipew Your work and fix is much appreciated. We run in this problem in our production system. To get this patch accepted, it needs some fix ups.
The patch don't follow all the PHP core coding standard https://github.com/php/php-src/blob/master/CODING_STANDARDS .
In some cases the indentation of php_openssl_store_errors(); is wrong.

Keep on with your good work.

@rdlowrey Can you check whether this looks good?

This seems sane to me. IMHO the openssl_error_string() functionality should never have been introduced to userland but that's water under the bridge at this point. The merge up for this from 5.5 won't be a ton of fun because the 5.6 extension code has a lot of changes from 5.5 and may require quite a few additional calls to the new php_openssl_store_errors() function.

@dol Ok, i will check and fix.
@rdlowrey Should i open another pull request to 5.6 branch ?

@dol Check last commit, i think the code is following the standards now.

@nikic Can you check it ? I'm already fixed the coding standards reported by @dol .

@wfelipew Thank you for taking the time of aligning the code. I think the spacing in some cases need to be fixed too . E.g.
tmp=OPENSSL_G( => tmp = OPENSSL_G(
}else{ => } else {

            err = emalloc(sizeof(PHP_SSL_ERROR_QUEUE));
            err->next=NULL;

@wfelipew I'm finishing up some userland work today but have the rest of the week blocked off to work on ext/openssl. I plan to merge this at that time and will let you know if I run into issues.

@rdlowrey can we get this merged please? I'm having users hit random pg_query failures over SSL'd connections because they also use openssl_… functions in their code... /cc @wfelipew

Also, @wfelipew, did you ever get a chance to submit a patch to libpq? They seemed happy to accept a patch: http://www.postgresql.org/message-id/flat/20150224030956.2529.83279@wrigleys.postgresql.org#20150224030956.2529.83279@wrigleys.postgresql.org

Need any help getting that in there? I can probably assist!

+1

I have got actually this on my list too if anyone else is quicker to address issues below.

I think that the idea is good. However the patch is a very uncomplete (missing lots places where we can have OpenSSL error) and not correct in some places ( e.g. php_openssl_open_base_dir_chk doesn't make much sense and php_openssl_evp_from_zval can also fail from other reasons). I know that extra checks won't do any harm but it's a bit messy IMHO. I also don't like some implementation details and CS is all over the place. :) It means that there is quite a bit work to do before it can be merged. I'd like to take a look this or next month and come up with something better that will then merge to 5.6+ (it can't go to 5.5 as it's not a security issue).

Hi @bukka, i will check the problems that you report on next days.

Just for the record. I have been working on an improved variant. It's still WIP and the diff can be seen here:

PHP-7.0...bukka:openssl_error_store

I need to add storing to many other places of course but the logic is in the same way how it works in OpenSSL. It means it never stores more then ERR_NUM_ERRORS for each thread. It is also more memory friendly. It also fixes some other problems that the patch in this PR has.

I decided to target 7.0 as I don't think this is really something wrong with openssl ext and it could possible cause some other issue if we miss some error storing or someone relies on getting errors from other then openssl ext. I think that this is a more feature for openssl ext and the real bug should be fixed in either in libpq or pgsql ext (clearing error queue before TLS IO operation). That would also fix the problem when error happens somewhere where it is not stored (e.g. some other extension using OpenSSL).

check that out ^^ @yohgaki :)

@bukka I think instead of php_openssl_store_errors, a name like php_openssl_move_errors or php_openssl_backup_errors would be better. That function declaration should probably also contain a long-ish comment with an explanation of why that is done, for future reference ;)

Have you had a good idea yet for testing this?

@dzuelke I'm thinking more about wrapping php_error_docref because often these two calls follow. I will think about it a bit more and come up with something later. But definitely good point! Thanks.

I'd like to separate this from the pgsql bug and more concentrate testing of openssl_error_string which should cover this functionality. Of course it's very difficult to cover all possible cases for all errors (quite a bit of work) which is also the reason why I based it on 7.0.

P.S. Not sure if your user is aware of it but there is actually a way how to clear the error queue in user space. I haven't tested it but from the code, something like this this should do:

while (openssl_error_string());

It's not an ideal solution but it might help your user ;)

Yeah I'm aware of that workaround @bukka but that'd have to be done before every query so it's not ideal...

@bukka I can confirm that the while (openssl_error_string()); works. We extended the database driver/adapter to perform this workaround before executing every query or interaction with the database. It's not ideal, but it will do the trick.

Have you checked what the impact on performance is, @dol?

@dzuelke No. But I guess the performance impact is negligible compared to the impacts that a sudden disconnect of your database connection will do to your application state.
Looking at the code of openssl_error_string() and get_error_values(OpenSSL) it will burn a few extra CPU cycles and memory access, but it's not looking very expensive

@dzuelke A faster workaround would be adding a new PHP function to clear the OpenSSL error stack with ERR_clear_error().

We created a fix for Postgres: http://www.postgresql.org/message-id/flat/CAM3SWZSOJ1p-6jE+h8iii6WboBmyFHuJto=S2Fk==y1wLV3pSQ@mail.gmail.com

Latest point releases were two days ago, and it did not make it in, so hopefully next time...

I'm going to close this PR as it is incomplete and outdated. I have been working on a better solution that I linked before ( master...bukka:openssl_error_store ) and managed to cover most of the cases where the error can be thrown since then. It needs quite a bit of testing however and possibly fixing issues. I'm getting there slowly...

Anyway the issue is more generic so it is better to address it just in openssl. I found another bug with soap that seems to be just partially fixed - https://bugs.php.net/bug.php?id=69882 . I haven't tested it but I guess that soap request over tls might fail if there is an error from other functions and not just pkcs12 but I would have to test it. There might be more similar cases with other extensions so it would be good idea to merge it to 7.0 once I'm done (if I manage to finish it till then of course :) ).

I can't actually close it as https://qa.php.net/pulls/ doesn't work for me. So I will leave it open :). It's probably better to keep it open for few more day in case someone thinks that it shouldn't be closed as yet...

A++ @bukka :) Feel free to CC me on a PR if you want me to test and/or review!

Fix in libpq committed to master and awaiting backport to maintenance version branches: http://www.postgresql.org/message-id/5707F49C.7010103@gmx.net

Comment on behalf of bukka at php.net:

Superseded by #1950

Just an update that I just created a PR that will merge after travis is fine. I decided to merge it just to the master as it cleans up the flow in terms of skipping some calls if it's clear that there are fails (e.g. if file BIO fails, we don't need to PEM loading with NULL bio). It has a small implication on the resulted errors (we don't get errors from PEM loading which is logical). Also the patch is quite big for bug fixing release and it's worthy it to go through the beta and RC stage. We are very soon to 7.1 anyway.