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

src: fix mismatched delete[] in src/node_file.cc #1092

Merged
merged 2 commits into from
Mar 7, 2015

Conversation

bnoordhuis
Copy link
Member

Fix a bad delete of a pointer that was allocated with placement new.
Casting the pointer was not the right solution because there was at
least one non-placement new constructor call.

This commit rewrites FSReqWrap to be more explicit about ownership of
the auxiliary data and removes a number of egregious const_casts.
The ASYNC_DEST_CALL macro also gets significantly slimmed down.

R=@indutny

Is there a bug number I should reference in the commit log?

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/257/

@indutny
Copy link
Member

indutny commented Mar 7, 2015

No open issues about it yet.

inline const char* dest() const { return dest_; }
inline unsigned int dest_len() const { return dest_len_; }
inline void dest_len(unsigned int dest_len) { dest_len_ = dest_len; }
void* operator new(size_t size) = delete;
Copy link
Member

Choose a reason for hiding this comment

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

How does this work?

Copy link
Member

Choose a reason for hiding this comment

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

Fix a bad delete of a pointer that was allocated with placement new.
Casting the pointer was not the right solution because there was at
least one non-placement new constructor call.

This commit rewrites FSReqWrap to be more explicit about ownership of
the auxiliary data and removes a number of egregious const_casts.
The ASYNC_DEST_CALL macro also gets significantly slimmed down.

PR-URL: nodejs#1092
Reviewed-By: Fedor Indutny <fedor@indutny.com>
The SYNC_CALL macro returns on error, bypassing the delete[] call.

Mea culpa, it looks like I introduced this memory leak back in 2013,
in commit d2b80b8 ("src: clean up FSReqWrap").

PR-URL: nodejs#1092
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@indutny
Copy link
Member

indutny commented Mar 7, 2015

LGTM

@bnoordhuis
Copy link
Member Author

@indutny I pushed another commit to fix a memory leak in the fs.writeSync() error path. Can you PTAL?

@bnoordhuis
Copy link
Member Author

Those SYNC_CALL and ASYNC_CALL macros are a special kind of evil. One of the first rules of C macros is that you don't push control flow into them and what do they do? Exactly.

return args.GetReturnValue().Set(SYNC_RESULT);
}

FSReqWrap* req_wrap =
new FSReqWrap(env, req.As<Object>(), "write", must_free ? buf : nullptr);
FSReqWrap::New(env, req.As<Object>(), "write", buf, ownership);
Copy link
Member

Choose a reason for hiding this comment

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

So you pas buf here, where does it die?

Copy link
Member

Choose a reason for hiding this comment

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

Anyway it seems that the buf should be deallocated somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's freed by FSReqWrap's destructor because ownership == MOVE.

@indutny
Copy link
Member

indutny commented Mar 7, 2015

One comment, otherwise LGTM

FSReqWrap* that;
char* const storage = new char[sizeof(*that) + size];
that = new(storage) FSReqWrap(env, req, syscall, data);
if (copy)
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I guess it becomes parts of FSReqWrap?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. :-)

@bnoordhuis bnoordhuis closed this Mar 7, 2015
@bnoordhuis bnoordhuis deleted the fix-bad-delete branch March 7, 2015 19:23
@bnoordhuis bnoordhuis merged commit 528d878 into nodejs:v1.x Mar 7, 2015
@rvagg rvagg mentioned this pull request Mar 8, 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.

2 participants