[asterisk-dev] [Code Review] Tab completion on optional arguments can get discombobulated when there is a command containing a substring of another command given the proper order

[jrose]

> On 2011-04-14 13:55:51, Russell Bryant wrote:
> > /branches/1.6.2/main/cli.c, lines 1756-1769
> > <https://reviewboard.asterisk.org/r/1180/diff/2/?file=16156#file16156line1756>
> >
> >     I'm a bit confused ... the strcasestr() was important in that it made sure what was typed actually matched something.  I understand the problem with substrings, but I'm not seeing the equivalent thing being done anymore.  Where is what the user typed being checked?

Yes, this is correct.  Turns out the if statement just checks the length of the command string to make sure it's the same as the length of the cli word...  which is a touch convoluted.  New diff uploading.


- jrose


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1180/#review3362
-----------------------------------------------------------


On 2011-04-13 15:50:15, jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1180/
> -----------------------------------------------------------
> 
> (Updated 2011-04-13 15:50:15)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> CLI tab completion can be confused by command options if there is some substring matching going on.  This has only been the case for commands once so far that I've actually been able to find, and it only existed in 1.6.2.
> 
> dahdi show channels has options [trunkgroup|group|context]
> 
> This example is tab completing against group.  If dahdi show channels group is in cli and the user tabs, the word group will be added again.  This can be repeated infinitely until the cli starts spitting argument errors.
> 
> 
> When the tab completion is being performed in ast_cli_generator, word_match is invoked using the cli word "group" against the command string [trunkgroup|group|context].  Inside this message, a string pointer pos is set using strcasestr(cli_word, cmd) which sets the cli command to the first occurence of the word in the command string.  Unfortunately this message is naive to the occurence of substrings, so the resulting pos for group is:
> 
> "group|group|context]"
> 
> instead of the expected "group|context]".
> 
> An if statement near the end of the word_match function evaluates the string to see if the word is a command word by checking if the character before it is a a cli reserved character (such as {, |, or [)
> 
> since the character before pos in this case is 'k', it skips this if clause and returns -1 instead of the expected 1, causing it to match back in the ast_cli_generator function.
> 
> 
> My fix for this is to search against the various reserved characters concatenated with the cli word.  Once one is found, another search is done on that substring to put it at the actual command so it is looking at the actual word rather than <reserved_character>word.  I think there is probably a way to search against all of these characters at once without having to rely on string concatenation, possibly with regular expressions, but I'm not especially certain on that and if anyone has any recommendations on that I'd be glad to follow them.
> 
> 
> This addresses bug 17494.
>     https://issues.asterisk.org/view.php?id=17494
> 
> 
> Diffs
> -----
> 
>   /branches/1.6.2/main/cli.c 313276 
> 
> Diff: https://reviewboard.asterisk.org/r/1180/diff
> 
> 
> Testing
> -------
> 
> Obviously a lot of tab completion on various inputs of cli words was used, but I also was using some fairly verbose checks of what was going on in the word_match function to make sure things were happening the way I think they were happening.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110414/6daa9750/attachment-0001.htm>