Bug+patch for tmux session lookup

[Natacha Porté]

Hi,

I think I have found a bug in the way tmux looks up sessions. Here is a
sample of the symptoms:

% tmux ls
foo1: 1 windows (created Mon Nov  2 13:18:01 2009) [80x24]
foo2: 1 windows (created Mon Nov  2 13:18:03 2009) [80x24]
foo: 1 windows (created Mon Nov  2 13:19:00 2009) [80x24]
bar: 1 windows (created Mon Nov  2 13:19:58 2009) [80x56] (attached)
% tmux attach-session -t foo
more than one session: foo
%

To reproduce the issue, you need a tmux server with at least three
sessions, one of them having a name being a prefix of at least two
sessions before it. In my example, "foo" is a prefix of "foo1" and
"foo2", both of them being before "foo".

Then trying to do anything with session foo will report an error of
"foo" being ambiguous, while in reality it is not, there is really a
session "foo".


This can be easily explained by the code of cmd_lookup_session() from
src/usr.bin/tmux/cmd.c. I am not an OpenBSD user, but from OpenBSD
cvsweb I checked the file with this version:
/* $OpenBSD: cmd.c,v 1.27 2009/10/26 21:42:04 deraadt Exp $ */

The function cmd_lookup_session() starts at line 483. In this function,
there is a for loop over the session names, when an exact match is found
the loop is prematurely exited by a return statement (line 501), and
when two prefix or pattern matches are found, *ambigous is set to 1.

The problem is that when two prefix/pattern matches are found BEFORE an
exact match, as in my testcase ("foo1" and "foo2" are prefix matches of
"foo"), *ambiguous is not set back to 0 when the exact match is
encountered.

My fix proposition is therefore to add *ambiguous = 0; before
return (s); in case of exact match. This is the content of the attached
patch.



Thanks for reading this mail,
Natacha Porti

[demime 1.01d removed an attachment of type text/x-diff]

[Nicholas Marriott-2]

Hi

Your diff didn't come through, but I don't think your fix is correct.

The problem is not that the ambiguous flag is not being set to zero - it is
always set to zero unless a multiple match happens.

The error is that tmux returns if there are two ambiguous matches, even if
there are subsequent full matches. They should be checked separately.

Please try this diff instead:


Index: cmd.c
===================================================================
RCS file: /cvs/src/usr.bin/tmux/cmd.c,v
retrieving revision 1.27
diff -u -p -r1.27 cmd.c
--- cmd.c 26 Oct 2009 21:42:04 -0000 1.27
+++ cmd.c 2 Nov 2009 14:43:52 -0000
@@ -488,19 +488,25 @@ cmd_lookup_session(const char *name, int
  *ambiguous = 0;
 
  /*
- * Look for matches. Session names must be unique so an exact match
- * can't be ambigious and can just be returned.
+ * Look for matches. First look for exact matches - session names must
+ * be unique so an exact match can't be ambigious and can just be
+ * returned.
  */
- sfound = NULL;
  for (i = 0; i < ARRAY_LENGTH(&sessions); i++) {
  if ((s = ARRAY_ITEM(&sessions, i)) == NULL)
  continue;
-
- /* Check for an exact match and return it if found. */
  if (strcmp(name, s->name) == 0)
  return (s);
-
- /* Then check for pattern matches. */
+ }
+
+ /*
+ * Otherwise look for partial matches, returning early if it is found to
+ * be ambiguous.
+ */
+ sfound = NULL;
+ for (i = 0; i < ARRAY_LENGTH(&sessions); i++) {
+ if ((s = ARRAY_ITEM(&sessions, i)) == NULL)
+ continue;
  if (strncmp(name, s->name, strlen(name)) == 0 ||
     fnmatch(name, s->name, 0) == 0) {
  if (sfound != NULL) {
@@ -510,7 +516,6 @@ cmd_lookup_session(const char *name, int
  sfound = s;
  }
  }
-
  return (sfound);
 }
 




On Mon, Nov 02, 2009 at 02:16:04PM +0100, Natacha Port? wrote:

> Hi,
>
> I think I have found a bug in the way tmux looks up sessions. Here is a
> sample of the symptoms:
>
> % tmux ls
> foo1: 1 windows (created Mon Nov  2 13:18:01 2009) [80x24]
> foo2: 1 windows (created Mon Nov  2 13:18:03 2009) [80x24]
> foo: 1 windows (created Mon Nov  2 13:19:00 2009) [80x24]
> bar: 1 windows (created Mon Nov  2 13:19:58 2009) [80x56] (attached)
> % tmux attach-session -t foo
> more than one session: foo
> %
>
> To reproduce the issue, you need a tmux server with at least three
> sessions, one of them having a name being a prefix of at least two
> sessions before it. In my example, "foo" is a prefix of "foo1" and
> "foo2", both of them being before "foo".
>
> Then trying to do anything with session foo will report an error of
> "foo" being ambiguous, while in reality it is not, there is really a
> session "foo".
>
>
> This can be easily explained by the code of cmd_lookup_session() from
> src/usr.bin/tmux/cmd.c. I am not an OpenBSD user, but from OpenBSD
> cvsweb I checked the file with this version:
> /* $OpenBSD: cmd.c,v 1.27 2009/10/26 21:42:04 deraadt Exp $ */
>
> The function cmd_lookup_session() starts at line 483. In this function,
> there is a for loop over the session names, when an exact match is found
> the loop is prematurely exited by a return statement (line 501), and
> when two prefix or pattern matches are found, *ambigous is set to 1.
>
> The problem is that when two prefix/pattern matches are found BEFORE an
> exact match, as in my testcase ("foo1" and "foo2" are prefix matches of
> "foo"), *ambiguous is not set back to 0 when the exact match is
> encountered.
>
> My fix proposition is therefore to add *ambiguous = 0; before
> return (s); in case of exact match. This is the content of the attached
> patch.
>
>
>
> Thanks for reading this mail,
> Natacha Porti
>
> [demime 1.01d removed an attachment of type text/x-diff]

[Nicholas Marriott-2]

Your fix would be right if they function didn't return on an ambiguous match,
just set the flag and continued the loop, but I think it is clearer to have two
loops anyway.


On Mon, Nov 02, 2009 at 02:46:09PM +0000, Nicholas Marriott wrote:

> Hi
>
> Your diff didn't come through, but I don't think your fix is correct.
>
> The problem is not that the ambiguous flag is not being set to zero - it is
> always set to zero unless a multiple match happens.
>
> The error is that tmux returns if there are two ambiguous matches, even if
> there are subsequent full matches. They should be checked separately.
>
> Please try this diff instead:
>
>
> Index: cmd.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/tmux/cmd.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 cmd.c
> --- cmd.c 26 Oct 2009 21:42:04 -0000 1.27
> +++ cmd.c 2 Nov 2009 14:43:52 -0000
> @@ -488,19 +488,25 @@ cmd_lookup_session(const char *name, int
>   *ambiguous = 0;
>  
>   /*
> - * Look for matches. Session names must be unique so an exact match
> - * can't be ambigious and can just be returned.
> + * Look for matches. First look for exact matches - session names must
> + * be unique so an exact match can't be ambigious and can just be
> + * returned.
>   */
> - sfound = NULL;
>   for (i = 0; i < ARRAY_LENGTH(&sessions); i++) {
>   if ((s = ARRAY_ITEM(&sessions, i)) == NULL)
>   continue;
> -
> - /* Check for an exact match and return it if found. */
>   if (strcmp(name, s->name) == 0)
>   return (s);
> -
> - /* Then check for pattern matches. */
> + }
> +
> + /*
> + * Otherwise look for partial matches, returning early if it is found to
> + * be ambiguous.
> + */
> + sfound = NULL;
> + for (i = 0; i < ARRAY_LENGTH(&sessions); i++) {
> + if ((s = ARRAY_ITEM(&sessions, i)) == NULL)
> + continue;
>   if (strncmp(name, s->name, strlen(name)) == 0 ||
>      fnmatch(name, s->name, 0) == 0) {
>   if (sfound != NULL) {
> @@ -510,7 +516,6 @@ cmd_lookup_session(const char *name, int
>   sfound = s;
>   }
>   }
> -
>   return (sfound);
>  }
>  
>
>
>
>
> On Mon, Nov 02, 2009 at 02:16:04PM +0100, Natacha Port? wrote:
> > Hi,
> >
> > I think I have found a bug in the way tmux looks up sessions. Here is a
> > sample of the symptoms:
> >
> > % tmux ls
> > foo1: 1 windows (created Mon Nov  2 13:18:01 2009) [80x24]
> > foo2: 1 windows (created Mon Nov  2 13:18:03 2009) [80x24]
> > foo: 1 windows (created Mon Nov  2 13:19:00 2009) [80x24]
> > bar: 1 windows (created Mon Nov  2 13:19:58 2009) [80x56] (attached)
> > % tmux attach-session -t foo
> > more than one session: foo
> > %
> >
> > To reproduce the issue, you need a tmux server with at least three
> > sessions, one of them having a name being a prefix of at least two
> > sessions before it. In my example, "foo" is a prefix of "foo1" and
> > "foo2", both of them being before "foo".
> >
> > Then trying to do anything with session foo will report an error of
> > "foo" being ambiguous, while in reality it is not, there is really a
> > session "foo".
> >
> >
> > This can be easily explained by the code of cmd_lookup_session() from
> > src/usr.bin/tmux/cmd.c. I am not an OpenBSD user, but from OpenBSD
> > cvsweb I checked the file with this version:
> > /* $OpenBSD: cmd.c,v 1.27 2009/10/26 21:42:04 deraadt Exp $ */
> >
> > The function cmd_lookup_session() starts at line 483. In this function,
> > there is a for loop over the session names, when an exact match is found
> > the loop is prematurely exited by a return statement (line 501), and
> > when two prefix or pattern matches are found, *ambigous is set to 1.
> >
> > The problem is that when two prefix/pattern matches are found BEFORE an
> > exact match, as in my testcase ("foo1" and "foo2" are prefix matches of
> > "foo"), *ambiguous is not set back to 0 when the exact match is
> > encountered.
> >
> > My fix proposition is therefore to add *ambiguous = 0; before
> > return (s); in case of exact match. This is the content of the attached
> > patch.
> >
> >
> >
> > Thanks for reading this mail,
> > Natacha Porti
> >
> > [demime 1.01d removed an attachment of type text/x-diff]

[Natacha Porté]

Le Monday 02 November 2009 ` 14:46, Nicholas Marriott icrivait:
> Hi
>
> Your diff didn't come through, but I don't think your fix is correct.
>
> The problem is not that the ambiguous flag is not being set to zero - it is
> always set to zero unless a multiple match happens.
>
> The error is that tmux returns if there are two ambiguous matches, even if
> there are subsequent full matches. They should be checked separately.

   Your diff probably yield the correct results, but I still believe my
patch to be correct and more efficient, because it only performs a
single pass on the session array.

   Here is my diff, verbatim, as attachments seem to be stripped away:

--- cmd.c-orig 2009-11-02 14:13:37.000000000 +0100
+++ cmd.c 2009-11-02 14:13:58.000000000 +0100
@@ -497,8 +497,10 @@
  continue;
 
  /* Check for an exact match and return it if found. */
- if (strcmp(name, s->name) == 0)
+ if (strcmp(name, s->name) == 0) {
+ *ambiguous = 0;
  return (s);
+ }
 
  /* Then check for pattern matches. */
  if (strncmp(name, s->name, strlen(name)) == 0 ||


   Because the first time we see an exact match, we *know* it is
unambiguous, no matter what happened before. Hence setting
unconditionnaly *ambiguous to zero yields the correct behaviour when
there is an exact match, and when there is no exact match the code is
still the same as before -- therefore as correct as before.

   When there is only a single prefix/pattern match, it is written is
sfound and *ambiguous is still at 0 (set at the beginning of the
function), which is correct, as it was before.

   When there is more than one prefix/pattern match, *ambiguous is set
to 1 inside the loop, and everything is correct too.

   So I fail to see in which case my patch is not correct, and I think
it's pretty clear why a single pass is more efficient than two passes.
As for code readability (which is also a desirable quality), I cannot
tell which patch is better.


Hoping this time it is clear enough,
Natacha Porti

[Natacha Porté]

Le Monday 02 November 2009 ` 14:54, Nicholas Marriott icrivait:
> Your fix would be right if they function didn't return on an ambiguous match,
> just set the flag and continued the loop, but I think it is clearer to have two
> loops anyway.

   I am sorry, but my fix does not change anything when there is only
ambiguous matches, because the only change is inside the exact-match
block, right before the return statement. So I still fail to see why my
fix would not be right.


Natacha Porti

[Nicholas Marriott-2]

Hi

Look at where the ambiguous variable is set in the function. It is assigned to
in two places:

- right at the start of the function it is set to zero;

- if an ambiguous match is found it is set to one then the function immediately
  returns.

So, there is no way for it to be non-zero except when an exact match is found
(where you add the assignment) - for the entire lifetime of the loop it is zero
except when it needs to be returned as one, and then the return is straight
after the assignment.

This means that your change has no effect, at the place where you add an
assignment to set *ambiguous to zero, it is already zero - it cannot become one
without the function returning.

We could use your solution if we also changed the loop to NOT return when
ambiguous is set to one, but continue the loop. Ie, do this as well as your
change:

                       if (sfound != NULL) {
                              *ambiguous = 1;
-                      return (NULL);
                       }

Doing two loops is not clearly more inefficient - consider that continuing the
loop would mean performing extra calls to strncmp and fnmatch, which may be
quite expensive. If this was something that needed to be time-critical it would
need to be considered more carefully, but it isn't, and I personally think two
loops is more readable here, although that may be subjective :-).

Can you test my change and see if it fixes the problem for you? It should apply
to the portable SF CVS as well at the moment.

Thanks


On Mon, Nov 02, 2009 at 04:07:58PM +0100, Natacha Port? wrote:

> Le Monday 02 November 2009 ? 14:54, Nicholas Marriott ?crivait:
> > Your fix would be right if they function didn't return on an ambiguous match,
> > just set the flag and continued the loop, but I think it is clearer to have two
> > loops anyway.
>
>    I am sorry, but my fix does not change anything when there is only
> ambiguous matches, because the only change is inside the exact-match
> block, right before the return statement. So I still fail to see why my
> fix would not be right.
>
>
> Natacha Port?

[Nicholas Marriott-2]

On Mon, Nov 02, 2009 at 03:49:40PM +0000, Nicholas Marriott wrote:

> Hi
>
> Look at where the ambiguous variable is set in the function. It is assigned to
> in two places:
>
> - right at the start of the function it is set to zero;
>
> - if an ambiguous match is found it is set to one then the function immediately
>   returns.
>
> So, there is no way for it to be non-zero except when an exact match is found

This should be s/except// of course, ie "no way for it to be non-zero when an
exact match is found".

> (where you add the assignment) - for the entire lifetime of the loop it is zero
> except when it needs to be returned as one, and then the return is straight
> after the assignment.
>
> This means that your change has no effect, at the place where you add an
> assignment to set *ambiguous to zero, it is already zero - it cannot become one
> without the function returning.
>
> We could use your solution if we also changed the loop to NOT return when
> ambiguous is set to one, but continue the loop. Ie, do this as well as your
> change:
>
>                        if (sfound != NULL) {
>                               *ambiguous = 1;
> -                      return (NULL);
>                        }
>
> Doing two loops is not clearly more inefficient - consider that continuing the
> loop would mean performing extra calls to strncmp and fnmatch, which may be
> quite expensive. If this was something that needed to be time-critical it would
> need to be considered more carefully, but it isn't, and I personally think two
> loops is more readable here, although that may be subjective :-).
>
> Can you test my change and see if it fixes the problem for you? It should apply
> to the portable SF CVS as well at the moment.
>
> Thanks
>
>
> On Mon, Nov 02, 2009 at 04:07:58PM +0100, Natacha Port? wrote:
> > Le Monday 02 November 2009 ? 14:54, Nicholas Marriott ?crivait:
> > > Your fix would be right if they function didn't return on an ambiguous match,
> > > just set the flag and continued the loop, but I think it is clearer to have two
> > > loops anyway.
> >
> >    I am sorry, but my fix does not change anything when there is only
> > ambiguous matches, because the only change is inside the exact-match
> > block, right before the return statement. So I still fail to see why my
> > fix would not be right.
> >
> >
> > Natacha Port?

[Natacha Porté]

Le Monday 02 November 2009 ` 15:49, Nicholas Marriott icrivait:
> This means that your change has no effect, at the place where you add an
> assignment to set *ambiguous to zero, it is already zero - it cannot become one
> without the function returning.
>
> We could use your solution if we also changed the loop to NOT return when
> ambiguous is set to one, but continue the loop. [...]

   Yes, you were right, I understand my mistake now. Thanks a lot for
having taken the time to explain.

> Can you test my change and see if it fixes the problem for you? It should apply
> to the portable SF CVS as well at the moment.

   Tested, it does work as far as I can see.


With all my thanks,
Natacha Porti

[Nicholas Marriott-2]

Great, committed. Thanks for the report!


On Mon, Nov 02, 2009 at 05:11:59PM +0100, Natacha Port? wrote:

> Le Monday 02 November 2009 ? 15:49, Nicholas Marriott ?crivait:
> > This means that your change has no effect, at the place where you add an
> > assignment to set *ambiguous to zero, it is already zero - it cannot become one
> > without the function returning.
> >
> > We could use your solution if we also changed the loop to NOT return when
> > ambiguous is set to one, but continue the loop. [...]
>
>    Yes, you were right, I understand my mistake now. Thanks a lot for
> having taken the time to explain.
>
> > Can you test my change and see if it fixes the problem for you? It should apply
> > to the portable SF CVS as well at the moment.
>
>    Tested, it does work as far as I can see.
>
>
> With all my thanks,
> Natacha Port?