search

phpseclib / phpseclib

PHP Secure Communications Library
http://phpseclib.com/
MIT License
5.34k stars 890 forks source link

Endless disconnect loop #2030

Closed Echron closed 1 month ago

Echron commented 1 month ago

I'm using version 3.0.41 on PHP 8.3. I'm uploading a bunch of images. The first go well, but after some time, the code gets stuck in an endless loop (and an out-of-memory error).

The error gets triggered in the method SSH2->disconnect_helper($reason) on the line with $this-send_binary_packet() the reason is always 10 (I believe connection lost). I'm gonna debug in a bit, but when I look into the method send_binary_packet() on the second line, $this->disconnect_helper is called again, and I see this method was changed a month ago in this commit: https://github.com/phpseclib/phpseclib/commit/51a3c5f050c8e319c5580d3c201db1a4117e3b96 and has been merged in 3.0.40

I'll debug later in the day to better understand what happens.

terrafrost commented 1 month ago

I believe this should be fixed in the latest git version. In particular, with 90eb0220af3c95563360efd03f750deaa90f6e61 .

Thanks!

Echron commented 1 month ago

Isn't this already merged in 3.0.41, which is the version that gives me the issues, hence downgrading to 3.0.39?

Something is strange, the file in your commit is showing me a completely different version than what I get when I install the package with composer (or even when I download it with from GitHub) for version 3.0.41 or even when I browse the master branch.

When I do that it says on top "PHP version 5", but when I look at the file in your commit is says "PHP versions 4 and 5".

image

When I look at a different commit for 3.0.41, for example https://github.com/phpseclib/phpseclib/pull/2023/commits/b94d55a73438ad40c0bcf55cf85f89f7c3fc2b63 then I see "PHP version 5" on top, and the file corrolates with the master and v3 branch.

So either I'm going completely mental (which is totally possible) or something is wrong with the commit you pointed to https://github.com/phpseclib/phpseclib/commit/90eb0220af3c95563360efd03f750deaa90f6e61 .

Echron commented 1 month ago

This is the file I see on the commit (not sure the URL will work for you) https://github.com/phpseclib/phpseclib/blob/90eb0220af3c95563360efd03f750deaa90f6e61/phpseclib/Net/SSH2.php

Echron commented 1 month ago

Never mind, this is the commit on branch 1.0 before you merged it into 3.0. I suspected it was me going mental.

However, the issue remains. This fix is available in 3.0.41, but that's the version with which I experienced the issue. I'll upgrade to 3.0.41 again and see if I can get in that catch statement before it goes wrong. It seems like the code you removed would actually stop the endless loop.

terrafrost commented 1 month ago

This fix is available in 3.0.41

Yah - you're right - sorry. I replied too early in the morning for me I guess lol.

Maybe you could do define('NET_SSH2_LOGGING', 3); at the top of the file, let it run for some amount of time, hit Ctrl + C, and then post the output, maybe on pastebin.com or some such?

rposky commented 1 month ago

I see how the issue described could be occurring, when the underlying socket has gone EOF. The disconnect_helper will attempt to send the SSH disconnect message and send_binary_packet will see the EOF and call back into disconnect_helper. Trying to put something together to address this.

Echron commented 1 month ago

Maybe you could do define('NET_SSH2_LOGGING', 3); at the top of the file, let it run for some amount of time, hit Ctrl + C, and then post the output, maybe on pastebin.com or some such?

I've done this when posting this, there is no SSH output at all. It's purely a loop in the code.

@rposky I didn't have much time to debug deeply, but it seems, indeed, like you say:

Let me know if you need more information or if I can debug something more targeted.

terrafrost commented 1 month ago

@Echron - if you could test out #2031 and let us know if that works for you that'd be great!

I'm at work atm but will try to review it either today or tomorrow as time permits.

Thanks!

Echron commented 1 month ago

Thanks! The code is running with the patch, I'll let you know what the results are. Have a good workday, and thx both of you for the quick actions.

Echron commented 1 month ago

@terrafrost, this patch works on my code base.

terrafrost commented 1 month ago

2031 has been merged.

Thanks for following up!