Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't infinite loop when searching CLI history list #2560

Merged
merged 2 commits into from
Dec 10, 2020

Conversation

seanbright
Copy link
Contributor

The easiest way to clean up the history list when moving an item is to
just remove it and reinsert, so use pj_list_erase().

Fixes #2559

The easiest way to clean up the history list when moving an item is to
just remove it and reinsert, so use pj_list_erase().

Fixes pjsip#2559
@CLAassistant
Copy link

CLAassistant commented Oct 23, 2020

CLA assistant check
All committers have signed the CLA.

… & updates: trim string before compare (as history entries are trimmed), replace pj_list_insert_nodes_after() with pj_list_erase(), avoid buffer overflow in string copy, and a bit optimization: only copy string when needed.
Copy link
Member

@sauwming sauwming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the spec and implementation of pj_strtrim() doesn't give me assurance that it's safe for zero-length pj_str that has uninitialised ptr.
So perhaps move the strtrim() below the if?

@nanangizz
Copy link
Member

The spec does not seem to specify anything about it, but looking at the implementation, the uninitalised ptr should never be dereferenced when the string is zero-length, so it should be safe? Or it is unsafe because of something else?

OTOH the original code seems to assume that the string length is always greater than zero: cmd.slen = pj_ansi_strlen(cmd_val)-1.

It should be better if the string can be trimmed first so we get a 'clean' string when evaluating the length (if (cmd.slen == 0)).

@sauwming
Copy link
Member

The code that's bothering me is:
register char *p = end - 1; which, theoretically, when the pj_str is zero length, allows the pointer to be outside the buffer and potentially underflow. Practically, it may be extremely rare, if possible at all, though.

Ah right, you want to check the length after trimming. I initially thought there's no point in doing so and that's why I suggest to move it below.

@nanangizz nanangizz merged commit 86f3ea6 into pjsip:master Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Telnet cli of pjsua runs into infinite loop after several new history entries
4 participants