Fix(LDAPObject): Prevent search attrlist memory error #555
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
tiran
requested changes
Jan 9, 2024
Member
tiran
left a comment
There was a problem hiding this comment.
Excellent finding. Thanks for the bug report and patch with tests. Much appreciated.
I suggest a slightly different fix to align the change with other upcoming changes.
d1358f6 to
0859ed8
Compare
Author
|
Sorry, lost track of this for a bit. I squashed the commits and adjusted the commit message. |
Contributor
|
@jkoeniger I think it should be good to merge. Could you please rebase it? |
Contributor
|
@droideck @jkoeniger is in vacation for the next two weeks. |
Contributor
|
@tiran needs to unset the |
be2210b to
1e46c73
Compare
Contributor
|
@droideck Rebase done. Can be merged. |
Add tests test_valid_attrlist_parameter_types and test_invalid_attrlist_parameter_types which test the behaviour when passing different Python types. All iterables which return only strings should pass, everything else should raise a TypeError.
Function `PySequence_Length` can return -1 on iterables like `dict`. The following PyMem_NEW still succeeds due `PyMem_NEW(char *, -1 + 1)` being equivalent to `char** PyMem_Malloc(1)`, which then can result in a segmentation fault later on. Solution: Use `seq` and `PySequence_Size` to determine the size of the sequence. This way any iterable which contains only strings can be used. Co-authored-by: Christian Heimes <christian@python.org>
1e46c73 to
450842d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Problem
Providing a dictionary to
attrlist(LDAPObject.search_ext) results in a memory error.See Issue #556
Root cause
Function
PySequence_Lengthcan return -1 on iterables likedict. The following PyMem_NEW still succeeds duePyMem_NEW(char *, -1 + 1)being equivalent tochar** PyMem_Malloc(1), which then can result in asegmentation fault later on.
Solution
The expected behavior should be either a raised
TypeErroror that any iterable which contains only strings can be passed.Commit e18c567 enables the latter which is in itself very unlikely to break anything. I would actually prefer to raise a
TypeErrorin the C extension on encountering a dictionary as it is most likely an error on the callers side, maybe we can add that later.The fix in e18c567 is to use
seqandPySequence_Fast_GET_SIZEto determine the size of the sequence. This way any iterable which contains only strings can be used. I added two tests in a1bdf47 which will fail without the fix in e18c567.