Re: [RFC] channels regression issue

[Jakub Jelen <jjelen@xxxxxxxxxx>]

On Thu, 2020-04-16 at 16:05 +0200, Heiko Thiery wrote:
> Hi Jakub and all,
> 
> > Tests are living in the tests/ directory. This does not sound like
> > something that could be covered with unit test so either client or
> > server directory would be appropriate. We do not have much code
> > coverage for this low-level channel handling so it is appropriate
> > to
> > create a new test based either of existing ones invoking this
> > timeout
> > and this bug. The testsuite already allows you to start openssh
> > servers
> > (most of the tests under client/) or invoke existing clients (for
> > example pkd/ tests).
> > 
> > Let me know if you will have some trouble putting things together.
> 
> --- a/tests/client/torture_session.c
> +++ b/tests/client/torture_session.c
> @@ -118,12 +118,45 @@ static void torture_channel_read_error(void
> **state) {
>      ssh_channel_free(channel);
>  }
> 
> +static void torture_channel_poll_timeout(void **state) {
> +    struct torture_state *s = *state;
> +    ssh_session session = s->ssh.session;
> +    ssh_channel channel;
> +    int rc;
> +    int fd;
> +    int i;
> +
> +    channel = ssh_channel_new(session);
> +    assert_non_null(channel);
> +
> +    rc = ssh_channel_open_session(channel);
> +    assert_ssh_return_code(session, rc);
> +
> +    fd = ssh_get_fd(session);
> +    assert_true(fd > 2);
> +
> +       rc = ssh_channel_poll_timeout(channel, 500, 0);
> +       assert_int_equal(rc, SSH_OK);
> +
> +    /* send crap and for server to send us a disconnect */
> +    rc = write(fd, "AAAA", 4);
> +    assert_int_equal(rc, 4);
> +
> +       rc = ssh_channel_poll_timeout(channel, 500, 0);
> +       assert_int_equal(rc, SSH_ERROR);
> +
> +    ssh_channel_free(channel);
> +}
> +
>  int torture_run_tests(void) {
>      int rc;
>      struct CMUnitTest tests[] = {
>          cmocka_unit_test_setup_teardown(torture_channel_read_error,
>                                          session_setup,
>                                          session_teardown),
> +        cmocka_unit_test_setup_teardown(torture_channel_poll_timeout
> ,
> +                                        session_setup,
> +                                        session_teardown),
>      };
> 
>      ssh_init();
> 
> is this somehow make sense here? frankly speaking I don't excactly
> know if this is the right position to do this test. A good case that
> returns valid data is missing.
> 
> What do you think?

I tested this with and without my patch in master and it looks like it
reproduces the issue just fine. The only nit is that the variable i is
unused in your patch and there is some indentation mismatch.

Please, submit it as a merge request on the gitlab so it will not get
lost here.

Thanks,
Jakub