Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mcrypt_generic(): is not a valid MCrypt resource #107

Closed
deviarte opened this issue May 13, 2013 · 18 comments
Closed

mcrypt_generic(): is not a valid MCrypt resource #107

deviarte opened this issue May 13, 2013 · 18 comments
Assignees

Comments

@deviarte
Copy link

Hi,

We are using phpseclib for our internal app deployment and we just updated to the latest version of the library (master branch) and everything seemed to be fine.

But on some servers something goes wrong.

The deployment itself works fine (connecting/mkdir/upload/delete) but it generated this PHP error:

Severity: Warning
Message: mcrypt_generic(): 343 is not a valid MCrypt resource
Filename: Crypt/RC4.php
Line Number: 360

When we revert back to the old version of the library, every single server works fine:
@Version $Id: SSH2.php,v 1.53 2010-10-24 01:24:30 terrafrost Exp $

Note: Using a password as the authentication method.

Not sure if i am providing enough information to properly debug this.

Thanks!

@bantu
Copy link
Member

bantu commented May 13, 2013

Please also provide your PHP version.

@deviarte
Copy link
Author

PHP Version 5.3.2-1ubuntu4.18

MCrypt:
Version 2.5.8
Api No 20021217
Supported ciphers cast-128 gost rijndael-128 twofish arcfour cast-256 loki97 rijndael-192 saferplus wake blowfish-compat des rijndael-256 serpent xtea blowfish enigma rc2 tripledes
Supported modes cbc cfb ctr ecb ncfb nofb ofb stream

@terrafrost
Copy link
Member

I'll try to take a look in the next few days. Had a super busy weekend. Will be in San Francisco for a week starting this weekend so there may be more delays too but I'll do what I can :)

@terrafrost
Copy link
Member

Oh - something that'd help - do you think you could provide an example that isolates the problem? Thanks!

@ghost ghost assigned bantu May 31, 2013
@bantu
Copy link
Member

bantu commented May 31, 2013

Unfortunately, the SSH2 file still says "@Version $Id: SSH2.php,v 1.53 2010-10-24 01:24:30 terrafrost Exp $" in it's current state, so it is not really clear which version you are using. The date maps to commit dd4f003.

@bantu
Copy link
Member

bantu commented May 31, 2013

This seems to be caused by the destructors of Net_SSH2 and Crypt_RC4. The PHP manual says

The destructor method will be called as soon as there are no other references to a particular object, or in any order during the shutdown sequence.

The last part seems to be important here and it looks like the Crypt_RC4 destructor is called before the Net_SSH2 destructor, Net_SSH2's destructor however uses Crypt_RC4 in send_binary_packet().

@phpsyscoder
Copy link
Member

Bantu, after your comment i was able to reproduce this error with the following script which will force the php engine to call Crypt_RC4::__destruct() before Crypt_SSH2::__destruct() on script_shutdown.

(Note: For Linux, i had to use $i < 11 instead of $i < 6 to trigger the error... for what php-internal reasons ever... maybe on your machine you have playing around with $i)

(Note: Ensure that the ssh server supports RC4... openssh will support it)

ini_set('error_reporting',E_ALL);
require_once('Net/SSH2.php');
require_once('Crypt/RC4.php');
define('CRYPT_RC4_MODE', CRYPT_RC4_MODE_MCRYPT);

// for ($i = 0; $i < 11; $i++) // on Linux
for ($i = 0; $i < 6; $i++) // on Windows 
{
    $idx = $i % 2;
    $ssh[$idx] = new Net_SSH2('192.168.184.129');
    $ssh[$idx]->login('testuser', '****************');
}
echo "exit(0)\n";
exit(0);

Output:

exit(0)
Warning: mcrypt_generic(): 325 is not a valid MCrypt resource in Crypt\RC4.php on line 360

It seems, that there are 2 possible ways to avoid this error.
removing Crypt_SSH2::__destruct()
or
removing Crypt_RC4::__destruct()

Personally... i don't like both ways :-) but in case of... i tend more to removing Crypt_RC4::__destruct() ...

Is there maybe one more way to fix it?

@bantu
Copy link
Member

bantu commented Jun 1, 2013

@Petrich Do you know whether calling mcrypt_module_close() is actually necessary? Are there any resources wasted if it is not called? The Net_SSH2 descructor seems to be more important because it closes a network connection, so I agree with you. It might be possible to force an order using register_shutdown_function or so, but it probably isn't straight forward.

@bantu
Copy link
Member

bantu commented Jun 1, 2013

Have to look at all other destructors as well, this might not be a problem with RC4 only.

@bantu
Copy link
Member

bantu commented Jun 1, 2013

Also, let us remove the outdated @version data generated by SVN.

@bantu
Copy link
Member

bantu commented Jun 1, 2013

Also, the PHP manual should probably clearify what happens when two object reference are cleared at the same time as seems to be the case here.

@bantu
Copy link
Member

bantu commented Jun 1, 2013

Grep for destruct:

./Net/SSH2.php:    function __destruct()
./Net/SSH1.php:    function __destruct()
./Crypt/RC4.php:    function __destruct()

@phpsyscoder
Copy link
Member

Do you know whether calling mcrypt_module_close() is actually necessary? Are there any resources wasted if it is not called?

It seems not. At least not in the context of __destruct()'ing. It seems that php closing the opened mcrypt module automatically if not needed anymore.

I wrote a small testscript which checks the memory usage.
Creates 1000 Crypt_RC4() objects over and over without calling mcrypt_module_close() by skipping Crypt_RC4::__destruct()

Memory stays stable, even after 1.000.000 rounds.

require_once('Crypt/RC4.php');
define('CRYPT_RC4_MODE', CRYPT_RC4_MODE_MCRYPT);

class Crypt_RC4_skipDestructor extends Crypt_RC4
{
    function __destruct()
    {
        return;
    }
}

if (!function_exists('memory_get_usage')) {function memory_get_usage(){};} // if not available, look at Linux::top or Windows::Taskmanager

for ($i = 0; true; ++$i) {
    $idx = $i % 1000;
    $cipher[$idx] = new  Crypt_RC4_skipDestructor();
    $cipher[$idx]->decrypt($cipher[$idx]->encrypt('a'));
    echo "[$i] [".str_pad($idx, 3)."] mem usage: " . memory_get_usage() . " bytes\n";
}

So it seems safe removing Crypt_RC4::__destruct()
Tested with php4 and 5.0 / 5.1 / 5.2 / 5.3 / 5.4 / 5.5 on win and *nix

@bantu
Copy link
Member

bantu commented Jun 1, 2013

Okay, in that case, let's just remove it. Want to do that?

@phpsyscoder
Copy link
Member

Yes. But want wait Terra's comment also... maybe we miss something. But this would fix the bug.

@phpsyscoder phpsyscoder mentioned this issue Jun 2, 2013
@ghost ghost assigned bantu and phpsyscoder Jun 2, 2013
@terrafrost
Copy link
Member

Removing Crypt_RC4's destructor works. Ugh I don't like the "[the destructor can be called in] any order during the shutdown sequence" behavior of PHP.

@phpsyscoder
Copy link
Member

Bantu, Terra... i removed the __destruct()'or in RC4

@deviarte This bug is fixed now. Could you download the current master-branch and let run your application with it? There should be no mcrypt_generic() warnings anymore.

Greetings :-)

@bantu
Copy link
Member

bantu commented Jun 4, 2013

Fixed for me. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants