Fix str_errors parameter regression in C extension (#255) #271
Conversation
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
Owner
|
This is addressed by an earlier open PR, #270, yes? |
## Summary Fixes agronholm#255 - Restores support for the `str_errors` parameter in the C extension when decoding CBOR strings containing invalid UTF-8 byte sequences. The parameter has been broken since v5.6.0. ## Problem Description Since version 5.6.0, the `str_errors` parameter in `cbor2.loads()` stopped working in the C extension (`_cbor2`). When decoding CBOR data containing invalid UTF-8 sequences: **Expected behavior (v5.5.1 and earlier):** ```python cbor2.loads(b"cfo\x90", str_errors="replace") # Returns: 'fo�' ``` **Broken behavior (v5.6.0 - v5.8.0):** ```python cbor2.loads(b"cfo\x90", str_errors="replace") # Raises: CBORDecodeValueError ``` The pure Python implementation (`cbor2._decoder`) continued to work correctly; only the C extension was affected. ## Root Cause **Commit 387755e (PR agronholm#204) - "Fixed MemoryError when decoding large definite strings"** Date: Jan 14, 2024 This commit rewrote the string decoding logic in `source/decoder.c` to handle large strings in chunks, fixing memory issues. However, it accidentally removed the `str_errors` parameter from UTF-8 decoding calls: **Before (v5.5.1 - working):** ```c ret = PyUnicode_DecodeUTF8(buf, length, PyBytes_AS_STRING(self->str_errors)); ``` **After (v5.6.0 - broken):** ```c // Short strings (≤65536 bytes) PyObject *ret = PyUnicode_FromStringAndSize(bytes, length); // No error handler support // Long strings (>65536 bytes) string = PyUnicode_DecodeUTF8Stateful(source_buffer, chunk_length, NULL, &consumed); // NULL instead of str_errors ``` ### Solution Re-add the missing parameter. ### Test Plan Add a new unit test for this functionality.
topher200
force-pushed
the
error-handling-fix
branch
from
25cbc6c to
ab091c7
Compare
Author
|
What a fun coincidence! I started on this PR last week, funny to see you fix it yesterday. Yes, it looks like your PR is a superset of mine. I confirmed that if I bring the test from this PR into the impl from your PR, it passes. I'll close this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Summary
Fixes #255 - Restores support for the
str_errorsparameter in the C extension when decoding CBOR strings containing invalid UTF-8 byte sequences. The parameter has been broken since v5.6.0.Problem Description
Since version 5.6.0, the
str_errorsparameter incbor2.loads()stopped working in the C extension (_cbor2). When decoding CBOR data containing invalid UTF-8 sequences:Expected behavior (v5.5.1 and earlier):
Broken behavior (v5.6.0 - v5.8.0):
The pure Python implementation (
cbor2._decoder) continued to work correctly; only the C extension was affected.Root Cause
Commit 387755e (PR #204) - "Fixed MemoryError when decoding large definite strings" Date: Jan 14, 2024
This commit rewrote the string decoding logic in
source/decoder.cto handle large strings in chunks, fixing memory issues. However, it accidentally removed thestr_errorsparameter from UTF-8 decoding calls:Before (v5.5.1 - working):
After (v5.6.0 - broken):
Changes
Re-add the missing parameter.
Fixes #255
Test Plan
Add a new unit test for this functionality.
Checklist
If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):
tests/) which would fail without your patchdocs/), in case of behavior changes or newfeatures
docs/versionhistory.rst).