Fix handling of truncated legacy errorpage %codes#2411
Fix handling of truncated legacy errorpage %codes#2411rousskov wants to merge 2 commits intosquid-cache:masterfrom
Conversation
noteBuildError_: ... Unsupported error page %code near % When build.input ends with a bare percent character, we must only copy/consume that character and increment build.input by 1, not 2. This overread bug existed since errorpage templates were introduced in 1997 commit 9b312a1. 2010 commit 4d16918 significantly broadened the kinds of use cases that can trigger this bug.
errorpage.cc(1294) compileLegacyCode: %? --> '%' Nothing was printed after the first '%' char...
rousskov
added
the
S-could-use-an-approval
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
There is another bug implied by @jro-calif report that triggered this pull request, but that bug is very different in nature and deserves a separate fix. I hope to post the corresponding pull request within 48 hours.
|
|
||
| case '\0': | ||
| // XXX: Partially duplicates error handling code of the `default:` case. | ||
| // TODO: Refactor bypassBuildErrorXXX() to accept `build` and determine the source of the error. |
There was a problem hiding this comment.
I am not addressing this XXX/TODO in this PR to provide a surgical fix that is easier to backport.
|
|
||
| Assure(build.input[1]); | ||
| mb.append(build.input, 2); | ||
| do_quote = 0; |
There was a problem hiding this comment.
We should not disable quoting in this case IMO, but that is a different problem that has a much smaller impact and that is best addressed when refactoring this code to fully address other related problems.
|
|
||
| debugs(4, 3, "%" << letter << " --> '" << p << "'" ); | ||
| // TODO: Add an I/O manipulator to report non-printable chars better. | ||
| debugs(4, 3, "%" << (letter ? letter : '?') << " --> '" << p << "'" ); |
There was a problem hiding this comment.
Without this hack, debugging line is truncated at the first % character. It would be better to report the NUL character instead of printing a somewhat misleading ?. This should be done while addressing the above manipulator TODO. I am not addressing this TODO in this PR to provide a surgical fix that is easier to backport.
yadij
added
M-cleared-for-merge
When build.input ends with a bare percent character, we must only copy/consume that character and increment build.input by 1, not 2. This overread bug existed since errorpage templates were introduced in 1997 commit 9b312a1. 2010 commit 4d16918 significantly broadened the kinds of use cases that can trigger this bug.
squid-anubis
added
the
M-waiting-staging-checks
squid-anubis
added
M-merged
|
queued for backport to v7 |
squidadm
removed
the
backport-to-v7
When build.input ends with a bare percent character, we must only copy/consume that character and increment build.input by 1, not 2. This overread bug existed since errorpage templates were introduced in 1997 commit 9b312a1. 2010 commit 4d16918 significantly broadened the kinds of use cases that can trigger this bug.
Finally done at #2416. It took me a few iterations to find the correct solution and polish it... |
5 participants
When build.input ends with a bare percent character, we must only
copy/consume that character and increment build.input by 1, not 2.
This overread bug existed since errorpage templates were introduced in
1997 commit 9b312a1. 2010 commit 4d16918 significantly broadened the
kinds of use cases that can trigger this bug.