Re: [RFC] channels regression issue

[Jakub Jelen <jjelen@xxxxxxxxxx>]

On Tue, 2020-04-21 at 11:25 +0200, Heiko Thiery wrote:
> Hi Jakub,
> 
> Am Di., 21. Apr. 2020 um 10:59 Uhr schrieb 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_err
> > > or,
> > >                                          session_setup,
> > >                                          session_teardown),
> > > +        cmocka_unit_test_setup_teardown(torture_channel_poll_tim
> > > eout
> > > ,
> > > +                                        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.
> 
> No problem. Do you have an idea what to do for checking the valid
> case
> with returned data?

What is done above is first waiting for timeout then breaking the
connection by sending invalid data and expecting the poll_timeout
fails, which is correct.

To get some use case with valid data, you will need to create a real
channel where you can pass some data and expect it will return some. I
think the easiest thing would be using a ssh_channel_request_exec()
with some common *nix commands that will work on most of the setups, as
explained in the tutorial:

http://api.libssh.org/stable/libssh_tutor_guided_tour.html#using_ssh

Obviously before doing the channel_read(), the channel_poll_timeout()
will need to be exercised.

Hope it helped,
-- 
Jakub Jelen
Senior Software Engineer
Security Technologies
Red Hat, Inc.