Skip to content

Fix NSData initWithBytesNoCopy:length:deallocator:#213

Merged
fredkiefer merged 2 commits into
gnustep:masterfrom
madsmtm:patch-1
Oct 29, 2021
Merged

Fix NSData initWithBytesNoCopy:length:deallocator:#213
fredkiefer merged 2 commits into
gnustep:masterfrom
madsmtm:patch-1

Conversation

@madsmtm
Copy link
Copy Markdown
Contributor

@madsmtm madsmtm commented Oct 29, 2021

The previous implementation simply swizzled NSData into NSDataWithDeallocatorBlock, and forgot to actually assign bytes and length. NSMutableData does not have this problem.

An alternative would be to call NSDataWithDeallocatorBlock's initWithBytesNoCopy:length:deallocator: at this point, I don't know which is better.

The previous implementation simply swizzled `NSData` into `NSDataWithDeallocatorBlock`, and forgot to actually assign `bytes` and `length`.
@madsmtm madsmtm requested a review from rfm as a code owner October 29, 2021 07:18
madsmtm added a commit to madsmtm/objc2 that referenced this pull request Oct 29, 2021
NSData::from_vec is broken in GNUStep, see gnustep/libs-base#213
madsmtm added a commit to madsmtm/objc2 that referenced this pull request Oct 29, 2021
NSData initWithBytesNoCopy:length:deallocator: is broken in GNUStep, see gnustep/libs-base#213
madsmtm added a commit to madsmtm/objc2 that referenced this pull request Oct 29, 2021
NSData initWithBytesNoCopy:length:deallocator: is broken in GNUStep, see gnustep/libs-base#213
Copy link
Copy Markdown
Member

@fredkiefer fredkiefer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As always it would be great to add a test for the bug you are fixing here. That would prevent the same bug from occurring again in the future.

Specifically on initWithBytesNoCopy:length:deallocator:
@madsmtm
Copy link
Copy Markdown
Contributor Author

madsmtm commented Oct 29, 2021

Alright, I think I did that (at least this test case is how I discovered the bug myself), but I'm open for suggestions, I'm not very experienced in your codebase.

@fredkiefer fredkiefer merged commit f958c37 into gnustep:master Oct 29, 2021
@madsmtm
Copy link
Copy Markdown
Contributor Author

madsmtm commented Oct 29, 2021

Cool, thanks for the super quick response!

@madsmtm madsmtm deleted the patch-1 branch October 29, 2021 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants