Ticket #96 (closed defect: fixed)

Opened 8 years ago

Last modified 7 years ago

Problems with remote port forwarding

Reported by: samikujala Owned by: bagder
Priority: normal Milestone:
Component: API Version:
Keywords: Cc: samikujala, bagder, roadrunn
Blocked By: Blocks:

Description

Out of curiosity (and to educate myself) I decided to try and set up remote port forwarding with using Libssh2. Something along the lines of

ssh -nNT -R 5900:localhost:5900 user@host

This turned out to be a non-trivial task due to my little experience with libssh2 and sparse documentation. To explain my findings, let's call my end host A, and the remote end host B. I wish to connect from host A to host B, after which the ssh daemon in host B is to establish remote port forwarding back to host A.

I was successful, but it appears that there are some bugs in channel.c file, which I address in the attached patch file. The way I establish the remote port forwarding, 'tcpip-forward' channel is as follows:

(assume the connection and session are established and user authentication is successful)

listener = libssh2_channel_forward_listen(session, 5900);
channel = libssh2_channel_forward_accept(listener);

  1. as packet_queue_listener() in packet.c successfully allocates and queues the incoming connection from host B into a new channel, _libssh2_packet_add() in packet.c fails to add the incoming packets into the proper channel, as _libssh2_channel_locate() in channel.c fails to locate the previously allocate channel.

If I understand correctly, this is because _libssh2_channel_locate() tries to locate the channels only from the current session, whereas the channel it is looking for is in the queue of incoming connections.

I fixed this by changing _libssh2_packet_add() to iterate not only through the current session, but also through the queues contained in the listeners contained in the current session.

  1. channel_forward_accept() in channel.c fails to break from the do-while loop because the condition to end the do-while loop is wrong. The condition (rc > 0) should read (rc < 0).

Please review my attached patch and if deemed correct, commit it into the tree.

Best regards,

Sami

Attachments

patch (1.3 KB) - added by samikujala 8 years ago.
Unified diff quick-fix patch for channel.c which solved the problem
ssh2_blocking.c (3.7 KB) - added by samikujala 8 years ago.
My test program to try remote port forwarding

Download all attachments as: .zip

Change History

Changed 8 years ago by samikujala

Unified diff quick-fix patch for channel.c which solved the problem

comment:1 Changed 8 years ago by bagder

Thanks for your patch.

Can you please tell me what that second part of the patch is supposed to fix? It will bail out the _libssh2_transport_read() loop in channel_forward_accept() if it returns something >= 0 and I would like to know what you think that's a good idea!

comment:2 Changed 8 years ago by samikujala

Hi,

Sorry for the late reply.

According to the code documentation, _libssh2_transport_read() returns the packet type ( > 0 ), PACKET_NONE, PACKET_FAIL, or PACKET_EAGAIN. The loop in channel_forward_accept() as it is now only ends when _libssh2_transport_read() reports failure. Please correct me if I'm wrong but would it not make more sense if loop would end when there are actual packets in the connection queue?

At least in my case I was not able to have channel_forward_accept() give me a channel upon remote connection before I changed the condition as it is in my patch.

Now that I'm thinking about it more, maybe the condition to end the loop should be ( rc <= 0 ) rather than ( rc < 0 ) to not end looping if we get PACKET_NONE from _libssh2_transport_read().

Regards,
Sami

comment:3 Changed 8 years ago by roadrunn

Is this another use case that was fixed with the patch from bug #2785173 (https://sourceforge.net/tracker/?func=detail&atid=703942&aid=2785173&group_id=125852)

comment:4 Changed 8 years ago by bagder

First, the _libssh2_transport_read() function can in fact return a few other errors too, but all errors (including EAGAIN) are negative values.

The PACKET_NONE is 0 and indicates that all is fine.

I figure the loop should then be (rc >= 0) so that it'll "drain" the incoming socket properly?

comment:5 Changed 8 years ago by bagder

Ahem. There's no need to loop when NONE is returned as then it didn't add anything. Thus > 0 is a fine check since then it loops only as long as there were packets actually added...

Changed 8 years ago by samikujala

My test program to try remote port forwarding

comment:6 Changed 8 years ago by samikujala

Ok,

I have probably misunderstood something. Attached is the test code I use to establish remote port forwarding for localhost. It's just modified version of the ssh2.c from the examples in the distribution. See below for sample run:

sami$ ./ssh2_blocking
[libssh2] 0.637253 Failure Event: -37 - Would block requesting userauth list

Authentication by public key succeeded.

[libssh2] 0.674195 Conn: Requesting tcpip-forward session for 127.0.0.1:6000
[libssh2] 0.675166 Failure Event: -37 - Would block
Got port 6000
[libssh2] 0.692373 Failure Event: -37 - Would block waiting for packet
[libssh2] 19.678288 Conn: Remote received connection from 127.0.0.1:54304 to 127.0.0.1:6000
[libssh2] 19.679719 Conn: Allocated new channel ID#0
[libssh2] 19.680448 Conn: Connection queued: channel 0/2 win 2097152/65536 packet 32768/32768
[libssh2] 19.681545 Failure Event: -37 - Would block waiting for packet
[libssh2] 19.682851 Conn: 1606 bytes packet_add() for 0/2/0
[libssh2] 19.685649 Failure Event: -37 - Would block waiting for packet
[libssh2] 25.409087 Conn: EOF received for channel 0/2
[libssh2] 25.410306 Failure Event: -37 - Would block waiting for packet
C

In the "remote end" I am just netcat'ing to port 6000, viz.
ls | nc localhost 6000

The libssh2 I use is snapshot version libssh2-1.1.1-20090507 with my patch applied WITHOUT the (rc < 0) part.

My operating system is macosx 10.4.11 on powerpc.

Libssh2 just won't break from the channel_forward_accept() loop. Everything works fine if I change the condition in channel_forward_accept() do-while loop.

Am I using the library wrong and if I am, I would appreciate any pointers towards right usage :)

comment:7 Changed 8 years ago by bagder

I'm not denying your problem, I'm just questioning the logic in that condition.

I've just now committed what's basically your patch, just written slightly different. I also made sure it sets another error if NULL is returned in a subsequent call and the reason is _not_ EAGAIN as I believe that might be the loop you see: it continuously believes the former error was EAGAIN even though it might not be.

Please update to latest CVS and see how your code runs now. I'll try to take your test code for a spin as well.

comment:8 Changed 7 years ago by bagder

  • Resolution set to fixed
  • Status changed from assigned to closed

No further reports in 9 months, I consider this case closed.

Note: See TracTickets for help on using tickets.