rposky
commented
4 years ago Thank you for your changes to php_ssh2_set_callback(). It helped me out of a bind.
Can you provide more context in support of the changes proposed to ssh2_disconnect()? They may not be necessary, and I think them undesirable, as I explain below.
Presently, if the libssh2 session resource is closed, libssh2_session_disconnect() will necessarily be invoked during resource destruction within php_ssh2_session_dtor(), the mechanics of which ensure libssh2_session_disconnect() is only invoked once, i.e. because a resource cannot be destructed twice.
By adding a call to libssh2_session_disconnect() from within ssh2_disconnect() and foregoing closing the session resource, the proposed changes allow libssh2_session_disconnect() to be invoked one or more times against any libssh2 session; once upon destruction and once anytime ssh2_disconnect() is called.
It may be the case that libssh2_session_disconnect() can tolerate multiple calls for the same libssh2 session. In either regard, I find the process less elegant than it exists today, particularly in the loss of guarantee that libssh2_session_disconnect() be invoked only once and also that resource destruction occur separately from ssh2_disconnect().
I've found that your proposed changes with the exception of those made within ssh2_disconnect() still resolves the segmentation fault potential that initially prompted my adoption of them. Perhaps there is another error being addressed that I am yet unaware of?
I've been able to use the following script to reproduce a segmentation fault issue about 50% of the time in PHP 7.2.9, and which is addressed by this change. The fault seems to arise when destructing the disconnect callback reference within php_ssh2_session_dtor() for the first libssh2 session resource and after key exchange activities overwrite the memory address allocated to the local $callbacks variable (or its 'disconnect' member) during the second loop iteration.
$onDisconnect = function ($reason, $message, $language) {
echo "In onDisconnect callback...\n";
};
for ($i = 0; $i < 2; $i++) {
$callbacks = ['disconnect' => $onDisconnect];
$connection = ssh2_connect('127.0.0.1', 22222, null, $callbacks);
if (!is_resource($connection)) {
die("Failed to connect");
}
if (!ssh2_auth_password($connection, 'user', 'pass')) {
die("Failed to authenticate with password");
}
}It would be great to see any scripts you have to recreate the problems you're addressing here. Thank you again for this pull request. Cheers.
Szpadel
Whissi
cmb69
langemeijer
Nice. When will the new release ?
Thanks to Szpadel