[Asterisk-code-review] cli: Fix infinite loop on terminating backslash (asterisk[master])

[N A]

Attention is currently required from: Kevin Harwell.
N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18004 )

Change subject: cli: Fix infinite loop on terminating backslash
......................................................................


Patch Set 1:

(2 comments)

File main/asterisk.c:

https://gerrit.asterisk.org/c/asterisk/+/18004/comment/1e486540_618b8b91 
PS1, Line 2929: 	if (ast_opt_remote) {
              : 		/*
              : 		* This fixes the bug where if the last char typed in
              : 		* the CLI before pressing TAB or ? is a backslash,
              : 		* then we get stuck in an infinite loop on the call
              : 		* to read in this function. This only affects remote
              : 		* console, so if we encounter a terminating backslash,
              : 		* get rid of it for the command completion, so the
              : 		* behavior is the same as foreground console. */
              : 		char *c = (char*) lf->buffer + strlen(lf->buffer) - 1;
              : 		if (*c == '\\') {
              : 			*c = '\0';
              : 		}
              : 	}
> I'm not entirely sure this is the correct fix for this. […]
Ack


https://gerrit.asterisk.org/c/asterisk/+/18004/comment/8d98db91_29a3f1aa 
PS1, Line 2988: 			res = read(ast_consock, mbuf + mlen, 1024);
> After a bit of testing here is where the code hangs. […]
Here is what is going on - I added log statements when the CLI process sends data to the main process, and whenever it reads something from the main process there.

pbxdev*CLI> te\sent: _COMMAND MATCHESARRAY "te\" "te\"
mbuf now: Usage: _command matchesarray "<line>" text
       This function is used internally to help with command completion and should.
       never be called by the user directly.

Compare this to a "normal" tab complete:
pbxdev*CLI> core shsent: _COMMAND MATCHESARRAY "core sh" "sh"
mbuf now: show show _EOF_
                   ow

So one suggestion that comes to mind is let the write to the main process happen as it does now, but if on the first read, we read something starting with "Usage: _command matchesarray" then we know something has gone wrong and we should abort and not continue reading.

Alternately, since it's waiting for AST_CLI_COMPLETE_EOF to appaer in the buffer before stopping the reads, it would seem that adding AST_CLI_COMPLETE_EOF to the usage for this internal command would make the most logical sense.

If I do the latter, then "Usage: " gets printed out to the CLI, which shouldn't happen either, but at least it doesn't block forever.

So that considered, maybe it does make sense to simply "do nothing", i.e. abort if on the first read, we get a "Usage:" - at least, I'm not sure what the sanest thing to do here would be. Thoughts?



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/18004
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I9810ac59304fec162da701653c9c834f0ec8f670
Gerrit-Change-Number: 18004
Gerrit-PatchSet: 1
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Wed, 18 May 2022 16:02:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220518/1b586a18/attachment.html>