Re: a small patch

[Xi Wang <xi.wang@xxxxxxxxx>]

Sorry for the mix of the patches...

I like your comments.  Thanks a lot!

- xi

On Sat, Apr 24, 2010 at 4:42 PM, Aris Adamantiadis <aris@xxxxxxxxxxxx> wrote:
> Hi Xi,
>
> Here are some comment about your patches. First of all, thank you for
> contributing to libssh.
> When possible, you should split the patches in small hunks so it's
> easier to select and comment.
> http://www.clearchain.com/blog/posts/splitting-a-patch gives a nice
> tool to do this.
>
>>
>> * libssh runs into a wrong state when connecting to a server that
>> supports keyboard-interactive (e.g., linux.mit.edu), "authenticated"
>> before asking for password.
>>
>> In ssh_userauth_kbdint (auth.c), first the code checks if
>> session->kbdint == NULL, then goes to ask_userauth to send
>> "ssh-userauth" and call ssh_handle_packets, where it will be
>> dispatched to ssh_packet_userauth_pk_ok.  Note that session->kbdint is
>> still NULL, session->auth_state will then be set to
>> SSH_AUTH_STATE_PK_OK, though it should have been SSH_AUTH_STATE_INFO.
>> One possible fix is to initialize session->kbinit right after the NULL
>> check.
>
> While I acknowledge the problem, I will not apply your patch, I think
> it's not the appropriate place to set the pointer. I will find a better
> solution, thanks for pointing this anyway.
>
>>
>> * two possible infinite loops in auth.c:218 and auth1.c:38, which can
>> be avoided by checking the return value of ssh_handle_packets.
> Applied
>>
>> * sample.c:451 calls ssh_userauth_none then later calls
>> authenticate_console.  actually authenticate_console will invoke
>> ssh_userauth_none again, which fails some server (e.g., bbs@xxxxxx).
> ssh_userauth_none is an important call at this step because it enables
> the banner that some servers send. I will move the fix back to
> ssh_userauth_none() which should not send twice the request.
>
>>
>> The attached patch also includes the fixes mentioned in my previous email.
>>
> in packet1.c, I moved the session state change in ssh_auth1_handler to
> avoid breaking the layering of the callback mechanism.
>
> So in short, I will rewrite the ssh_userauth_none and
> ssh_userauth_kbdint() patches. Thanks for your contribution.
>
> Aris
>> -xi
>>
>> On Wed, Apr 21, 2010 at 5:08 AM, Aris Adamantiadis <aris@xxxxxxxxxxxx> wrote:
>>> Hi Xi,
>>>
>>> Thanks for your patch. Libssh master is currently in architectural
>>> rework and SSH1 has been a little less polished than the rest. I'm happy
>>>  you managed to make it work.
>>>
>>> I have yet to verify that the \r\n -> \n thing does not break anything
>>> on openssh and cisco SSH.
>>>
>>> Regards,
>>>
>>> Aris
>>>
>>> Xi Wang a écrit :
>>>> Cool.  Here goes a few more fixes when I was trying libssh with
>>>> guest@xxxxxxxxxxx, an ssh-based forum using v1.
>>>>
>>>> * banner/identification: ssh_send_banner (client.c).  libssh reports
>>>> crc error after sending the session key.  The actual problem is that
>>>> libssh uses "...\r\n" in its identification, while some sever doesn't
>>>> parse the identification correctly and stops at "\r".  Since the ssh
>>>> specification (http://www.snailbook.com/docs/protocol-1.5.txt) says it
>>>> should be "...\n", the patch removes "\r".
>>>>
>>>> * hang (packet1.c:337).  libssh hangs after authentication.  I guess
>>>> it's because libssh doesn't run into the correct state, i.e., from
>>>> SSH_SESSION_STATE_AUTHENTICATING to SSH_SESSION_STATE_AUTHENTICATED.
>>>> Not sure if the fix is at the right place.
>>>>
>>>> * close: ssh_connection_callback (client.c:645).  When in error state
>>>> (e.g., the remote server closes the connection), libssh/samplessh
>>>> doesn't close automatically.  Not sure if this fix is necessary.
>>>>
>>>> * samplessh hangs (sample.c), for example, when channel_poll returns -1 (error).
>>>>
>>>> Thanks.
>>>>
>>>> - xi
>>>>
>>>> On Tue, Apr 20, 2010 at 7:32 AM, Andreas Schneider <mail@xxxxxxxxxxxx> wrote:
>>>>> On Tuesday 20 April 2010 13:05:45 you wrote:
>>>>>> Hi,
>>>>> Hi Xi,
>>>>>
>>>>>> Attached is a patch that addresses the following problems.
>>>>>>
>>>>>> * two memory leaks: enc_session (kex.c:781), session->packet_callbacks
>>>>>> (session.c:209);
>>>>>>
>>>>>> * openssl configuration problem: use OPENSSL_FOUND instead of
>>>>>> CRYPTO_FOUND, which doesn't exist in my cmake 2.8.
>>>>>>
>>>>>> * compile warnings: avoid using the name "signal" (channels.c);
>>>>>>
>>>>>> * link error when WITH_SERVER=0 (packet.c:59);
>>>>>>
>>>>>> * minor compile error (torture_options.c).
>>>>> thank you very much for your patch. I've split it up in smaller patches and
>>>>> pushed it to the repository.
>>>>>
>>>>>
>>>>> Cheers,
>>>>>
>>>>>        -- andreas
>>>>>
>>>>>
>>>>>
>>>
>>>
>
>