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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Mar 7, 2015
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
648fc63
src: fix mismatched delete[] in src/node_file.cc
bnoordhuis Mar 7, 2015
528d878
src: fix memory leak in fs.writeSync error path
bnoordhuis Mar 7, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 67 additions & 41 deletions src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,40 +49,72 @@ using v8::Value;

class FSReqWrap: public ReqWrap<uv_fs_t> {
public:
void* operator new(size_t size) { return new char[size]; }
void* operator new(size_t size, char* storage) { return storage; }
enum Ownership { COPY, MOVE };

inline static FSReqWrap* New(Environment* env,
Local<Object> req,
const char* syscall,
const char* data = nullptr,
Ownership ownership = COPY);

inline void Dispose();

void ReleaseEarly() {
if (data_ != inline_data()) {
delete[] data_;
data_ = nullptr;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to self: move the method body out of the class before landing.


const char* syscall() const { return syscall_; }
const char* data() const { return data_; }

private:
FSReqWrap(Environment* env,
Local<Object> req,
const char* syscall,
char* data = nullptr)
: ReqWrap(env, req, AsyncWrap::PROVIDER_FSREQWRAP),
syscall_(syscall),
data_(data),
dest_len_(0) {
const char* data)
: ReqWrap(env, req, AsyncWrap::PROVIDER_FSREQWRAP),
syscall_(syscall),
data_(data) {
Wrap(object(), this);
}

void ReleaseEarly() {
if (data_ == nullptr)
return;
delete[] data_;
data_ = nullptr;
}
~FSReqWrap() { ReleaseEarly(); }

inline const char* syscall() const { return syscall_; }
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.

Nvm, read http://en.wikipedia.org/wiki/C++11#Explicitly_defaulted_and_deleted_special_member_functions

void* operator new(size_t size, char* storage) { return storage; }
char* inline_data() { return reinterpret_cast<char*>(this + 1); }

private:
const char* syscall_;
char* data_;
unsigned int dest_len_;
char dest_[1];
const char* data_;

DISALLOW_COPY_AND_ASSIGN(FSReqWrap);
};


FSReqWrap* FSReqWrap::New(Environment* env,
Local<Object> req,
const char* syscall,
const char* data,
Ownership ownership) {
const bool copy = (data != nullptr && ownership == COPY);
const size_t size = copy ? 1 + strlen(data) : 0;
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. :-)

that->data_ = static_cast<char*>(memcpy(that->inline_data(), data, size));
return that;
}


void FSReqWrap::Dispose() {
this->~FSReqWrap();
delete[] reinterpret_cast<char*>(this);
}


static void NewFSReqWrap(const FunctionCallbackInfo<Value>& args) {
CHECK(args.IsConstructCall());
}
Expand Down Expand Up @@ -111,13 +143,12 @@ static void After(uv_fs_t *req) {

if (req->result < 0) {
// An error happened.
const char* dest = req_wrap->dest_len() > 0 ? req_wrap->dest() : nullptr;
argv[0] = UVException(env->isolate(),
req->result,
req_wrap->syscall(),
nullptr,
req->path,
dest);
req_wrap->data());
} else {
// error value is empty or null for non-error.
argv[0] = Null(env->isolate());
Expand Down Expand Up @@ -212,7 +243,7 @@ static void After(uv_fs_t *req) {
req_wrap->MakeCallback(env->oncomplete_string(), argc, argv);

uv_fs_req_cleanup(&req_wrap->req_);
delete req_wrap;
req_wrap->Dispose();
}

// This struct is only used on sync fs calls.
Expand All @@ -225,20 +256,10 @@ struct fs_req_wrap {
};


#define ASYNC_DEST_CALL(func, req, dest_path, ...) \
#define ASYNC_DEST_CALL(func, req, dest, ...) \
Environment* env = Environment::GetCurrent(args); \
FSReqWrap* req_wrap; \
char* dest_str = (dest_path); \
int dest_len = dest_str == nullptr ? 0 : strlen(dest_str); \
char* storage = new char[sizeof(*req_wrap) + dest_len]; \
CHECK(req->IsObject()); \
req_wrap = new(storage) FSReqWrap(env, req.As<Object>(), #func); \
req_wrap->dest_len(dest_len); \
if (dest_str != nullptr) { \
memcpy(const_cast<char*>(req_wrap->dest()), \
dest_str, \
dest_len + 1); \
} \
FSReqWrap* req_wrap = FSReqWrap::New(env, req.As<Object>(), #func, dest); \
int err = uv_fs_ ## func(env->event_loop(), \
&req_wrap->req_, \
__VA_ARGS__, \
Expand Down Expand Up @@ -811,7 +832,7 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
char* buf = nullptr;
int64_t pos;
size_t len;
bool must_free = false;
FSReqWrap::Ownership ownership = FSReqWrap::COPY;

// will assign buf and len if string was external
if (!StringBytes::GetExternalParts(env->isolate(),
Expand All @@ -824,22 +845,27 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
// StorageSize may return too large a char, so correct the actual length
// by the write size
len = StringBytes::Write(env->isolate(), buf, len, args[1], enc);
must_free = true;
ownership = FSReqWrap::MOVE;
}
pos = GET_OFFSET(args[2]);
req = args[4];

uv_buf_t uvbuf = uv_buf_init(const_cast<char*>(buf), len);

if (!req->IsObject()) {
// SYNC_CALL returns on error. Make sure to always free the memory.
struct Delete {
inline explicit Delete(char* pointer) : pointer_(pointer) {}
inline ~Delete() { delete[] pointer_; }
char* const pointer_;
};
Delete delete_on_return(ownership == FSReqWrap::MOVE ? buf : nullptr);
SYNC_CALL(write, nullptr, fd, &uvbuf, 1, pos)
if (must_free)
delete[] buf;
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.

int err = uv_fs_write(env->event_loop(),
&req_wrap->req_,
fd,
Expand Down