Forbid \C in UTF-8 patterns - #21139
Conversation
| coptions |= PCRE2_UCP; | ||
| #endif | ||
| /* The \C escape sequence is unsafe in PCRE2_UTF mode */ | ||
| coptions |= PCRE2_NEVER_BACKSLASH_C; |
There was a problem hiding this comment.
would need to document it somehow (UPGRADING ?) wdyt ?
| } | ||
| pcre2_get_error_message(errnumber, error, sizeof(error)); | ||
| if (errnumber == PCRE2_ERROR_BACKSLASH_C_CALLER_DISABLED) { | ||
| strlcpy((char*)error, "using \\C is incompatible with the 'u' modifier", sizeof(error)); |
There was a problem hiding this comment.
yes the default message is a bit vague in this context, makes sense.
|
I know enough pcre2 to see it s LGTM but cc @ndossche just in case :) |
|
Since it can otherwise induce some potentially unsafe behaviour, I think 8.4 is reasonable. |
|
Thank you @devnexen @ndossche. I figured that this may require the validation of @php/release-managers-84 @php/release-managers-85 too. TL;DR:
A quick grep of |
|
No objection from my side. I cut 8.5.5RC1 yesterday, but I'm happy to include this in 8.5.5 and take care of the communication if needed (Should the release be marked as a security release?) And asking for the release announcement: Is there a workaround we recommend? |
|
Thank you @edorian. A possible workaround would be to not use the Regarding inclusion in 8.5.5: It would be nice but not absolutely necessary. It would make sense to also include the fix in 8.4.20 in this case. Either way is fine with me, I let you decide. |
|
@arnaud-lb Great! If @php/release-managers-84 are good with it as well I'm happy to go with the upcoming release. Will you take of the merging into 8.5.5? Otherwises I'll cherrry-pick it after you've merged into 8.4 and up. |
|
Update: Given I didn't hear from @php/release-managers-84, some private feedback from folks, and that the chance isn't in 8.4 yet I didn't want to cherry-pick it into 8.5. So it'll go out with 8.5.6RC1 as a regular change. |
|
I doubt you all forget this PR :) Is this mergeable now? @arnaud-lb |
|
Since we didn't hear from @php/release-managers-84, I'm going to merge this in master only, soon |
|
Sorry I didn't see the email earlier, this seems OK to me if 8.5 RMs are also OK with it. |
DanielEScherzer
left a comment
DanielEScherzer
left a comment
There was a problem hiding this comment.
No objections from 8.5 RMs
RM approval for including in 8.5, technical review not performed
* PHP-8.5: Forbid \C in UTF-8 patterns (#21139)
* PHP-8.4: Forbid \C in UTF-8 patterns (php#21139)
8 participants
Possible fix for GH-21134.
\Cresults in undefined, potentially unsafe behavior in UTF-8 mode, so error out in case the escape sequence is used in UTF-8 mode.