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

Fix double-free on ternary operator parsing error #847

Merged
merged 3 commits into from Sep 20, 2019

Conversation

pmatilai
Copy link
Member

v1 gets double-freed when there's an error parsing the result side
of the ternary operator. Add testcases to go, one that catches the
actual error and another which serves to demonstrate that the false
branch doesn't get parsed at all, errors or no.

@mlschroe
Copy link
Contributor

good catch, thanks!

@mlschroe
Copy link
Contributor

The false branch does get parsed, this needs to be fixed as well.

@pmatilai
Copy link
Member Author

Oh, right, got mixed up trying various different things. With eg bare words (which is where I spotted this) both the true and false clauses report the error, but with unexpected end of expression the error message is missing (but it returns with exit code 1). I'll fix the test + commit message...

v1 gets double-freed when there's an error parsing the result side
of the ternary operator. Add testcases to go.
@mlschroe
Copy link
Contributor

I think we just need a

exprErr(state, _("syntax error in expression"), state->p);

in doPrimary()'s default case to fix missing error for %{expr:0 < 1 ? 4+ : 0}.

@pmatilai
Copy link
Member Author

pmatilai commented Sep 19, 2019

That'd work, but then we have inconsistent error messages depending on which branch the error is in:

$ ./rpm --eval "%{expr:0 < 1 ? 0 : 4+}"
error: unexpected end of expression: 0 < 1 ? 0 : 4+
$ ./rpm --eval "%{expr:0 < 1 ? 4+ : 0}"
error: syntax error in expression: 0 < 1 ? 4+ : 0
error:                                        ^

It also causes a duplicate error at least in the bareword case:

$ ./rpm --eval "%{expr:bare}"
error: bare words are no longer supported, please use "...": bare
error:                                                       ^
error: syntax error in expression: bare
error:                            ^

Adding TOK_TERNARY_ALT as a fallthrough case to TOK_EOF in doPrimary() seems to work but didn't actually think about it so dunno if there would be other side-effects.

@mlschroe
Copy link
Contributor

The duplicate error is because rpmExprStr/rpmExprBool ignore the return value of the
rdToken() call. That should also be fixed.

It's not really about TOK_TERNARY_ALT, you'll get the same error with %{expr:4 * +}

If the different error message bothers you, just delete the TOK_EOF case so that you'll
also get a syntax error if the expression ends unexpectedly.

@pmatilai
Copy link
Member Author

Added the rdToken() checks + the generic syntax error. Inconsistent error is much better than no error message at all...

@pmatilai pmatilai merged commit f008f8c into rpm-software-management:master Sep 20, 2019
@pmatilai pmatilai deleted the expr-doublefree-pr branch September 25, 2019 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants