gh-149079: Fix O(n^2) canonical ordering in unicodedata.normalize()#149080
Open
sethmlarson wants to merge 1 commit intopython:mainfrom
Open
gh-149079: Fix O(n^2) canonical ordering in unicodedata.normalize()#149080sethmlarson wants to merge 1 commit intopython:mainfrom
sethmlarson wants to merge 1 commit intopython:mainfrom
Conversation
…ze() Replace the insertion sort used for canonical ordering of combining characters with a hybrid approach: insertion sort for short runs (< 20) and counting sort for longer runs, reducing worst-case complexity from O(n^2) to O(n). This prevents denial of service via crafted Unicode strings with many combining characters in alternating CCC order. Co-authored-by: Seokchan Yoon <13852925+ch4n3-yoon@users.noreply.github.com>
sethmlarson
added
type-security
Member
|
Reviewers: Note that there are pending changes from previous reviews. |
maurycy
reviewed
Apr 27, 2026
| self.assertEqual(self.db.normalize('NFC', a), b) | ||
|
|
||
| def test_long_combining_mark_run(self): | ||
| # GH-XXXXX: avoid quadratic canonical ordering. |
Contributor
There was a problem hiding this comment.
- # GH-XXXXX: avoid quadratic canonical ordering.
+ # gh-149079: avoid quadratic canonical ordering.| self.assertEqual(self.db.normalize("NFKC", payload), nfc) | ||
|
|
||
| def test_combining_mark_run_fast_paths(self): | ||
| # GH-XXXXX: cover short runs and already-sorted long runs. |
Contributor
There was a problem hiding this comment.
- # GH-XXXXX: cover short runs and already-sorted long runs.
+ # gh-149079: cover short runs and already-sorted long runs.|
|
||
| if (run_length > sortbuflen) { | ||
| Py_UCS4 *new_sortbuf = PyMem_Realloc(sortbuf, | ||
| run_length * sizeof(Py_UCS4)); |
Contributor
There was a problem hiding this comment.
Maybe PyMem_Resize instead of calculating manually?
Lines 58 to 60 in 005555a
serhiy-storchaka
approved these changes
Apr 28, 2026
Member
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
There is a potential for optimization, but in general LGTM. 👍
| Py_ssize_t i, o, osize; | ||
| int kind; | ||
| const void *data; | ||
| int input_kind, result_kind; |
Member
There was a problem hiding this comment.
Why not reuse the same variable?
Member
There was a problem hiding this comment.
IIRC, I asked to have two different variables for readability purposes. We could reuse it but when reading the code, it was cleaner when I saw the separation. But it can be reverted if you insist.
| data = PyUnicode_DATA(result); | ||
| result_kind = PyUnicode_KIND(result); | ||
| result_data = PyUnicode_DATA(result); | ||
| length = PyUnicode_GET_LENGTH(result); |
Member
|
Ideas for optimization:
|
picnixz
reviewed
Apr 28, 2026
Comment on lines
+592
to
+608
| for (Py_ssize_t i = start; i < end; i++) { | ||
| Py_UCS4 code = PyUnicode_READ(kind, data, i); | ||
| unsigned char combining = _getrecord_ex(code)->combining; | ||
| counts[combining]++; | ||
| if (combining < min_combining) { | ||
| min_combining = combining; | ||
| } | ||
| if (combining > max_combining) { | ||
| max_combining = combining; | ||
| } | ||
| } | ||
|
|
||
| for (Py_ssize_t i = min_combining; i <= max_combining; i++) { | ||
| Py_ssize_t count = counts[i]; | ||
| counts[i] = total; | ||
| total += count; | ||
| } |
Member
There was a problem hiding this comment.
So we can drop the min/max stuff as I suggest privately.
picnixz
reviewed
Apr 28, 2026
| int needs_sort = 0; | ||
|
|
||
| prev = _getrecord_ex( | ||
| PyUnicode_READ(result_kind, result_data, i))->combining; |
Member
There was a problem hiding this comment.
And here, for readability, I suggested storing PyUnicode_READ() in a temp var (same some lines below).
picnixz
reviewed
Apr 28, 2026
| @@ -0,0 +1,5 @@ | |||
| Fix a potential denial of service in :func:`unicodedata.normalize`. The | |||
| canonical ordering step of Unicode normalization used an O(n²) insertion | |||
Member
There was a problem hiding this comment.
Suggested change
| canonical ordering step of Unicode normalization used an O(n²) insertion | |
| canonical ordering step of Unicode normalization used a quadratic-time insertion |
I suggested to avoid possible rendering issues (we also say "linear-time" afterwards)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace the insertion sort used for canonical ordering of combining characters with a hybrid approach: insertion sort for short runs (< 20) and counting sort for longer runs, reducing worst-case complexity from O(n^2) to O(n). This prevents denial of service via crafted Unicode strings with many combining characters with a large number of inversions in combing class order.
unicodedata.normalize("NFC")canonical ordering #149079