Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

smalloc: don't mix malloc() and new char[] #1205

Merged
merged 1 commit into from
Mar 19, 2015

Conversation

bnoordhuis
Copy link
Member

It's technically undefined behavior to mix malloc with delete[] and
new char[] with free(). smalloc was using new char[] in one place and
malloc() in another but in both cases the memory was freed with free().

R=@trevnorris

@trevnorris
Copy link
Contributor

Thanks. Can you add the following just below the PR-URL: line:

Fixes: 8f3f9f78 "smalloc: initial implementation"

LGTM

@bnoordhuis
Copy link
Member Author

Do you use the Fixes: tag for that? I only paste links there.

@Fishrock123
Copy link
Contributor

I thought fixes: was for issue links, and regular comments for origin commits?

@bnoordhuis
Copy link
Member Author

That's how I use it, yes. By the way, it looks like the buglet wasn't introduced until some time after 8f3f9f7; that commit uses only new and delete[].

@trevnorris
Copy link
Contributor

@bnoordhuis I've used "Fixes" for both linking issues and commits. Doesn't matter honestly.

It's technically undefined behavior to mix malloc with delete[] and
new char[] with free().  smalloc was using new char[] in one place and
malloc() in another but in both cases the memory was freed with free().

PR-URL: nodejs#1205
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@bnoordhuis bnoordhuis force-pushed the fix-smalloc-malloc-new-confusion branch from ffe2506 to 2034137 Compare March 19, 2015 21:35
@bnoordhuis bnoordhuis closed this Mar 19, 2015
@bnoordhuis bnoordhuis deleted the fix-smalloc-malloc-new-confusion branch March 19, 2015 21:35
@bnoordhuis bnoordhuis merged commit 2034137 into nodejs:v1.x Mar 19, 2015
@rvagg rvagg mentioned this pull request Mar 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants