Skip to content

Commit

Permalink
fix(windows): Fix read & write bugs for windows (#1364)
Browse files Browse the repository at this point in the history
* Fix OVERLAPPED initialization
* Fix buffer overrun for writing and reading
* Remove unsafe memset for non-POD structures
* Fix thread handle leak
  • Loading branch information
kimushu authored and reconbot committed Oct 16, 2017
1 parent 6ed7b3b commit 0e4b1f9
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 12 deletions.
7 changes: 0 additions & 7 deletions src/serialport.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ NAN_METHOD(Open) {
}

OpenBaton* baton = new OpenBaton();
memset(baton, 0, sizeof(OpenBaton));
strcpy(baton->path, *path);
baton->baudRate = getIntFromObject(options, "baudRate");
baton->dataBits = getIntFromObject(options, "dataBits");
Expand Down Expand Up @@ -125,7 +124,6 @@ NAN_METHOD(Update) {
}

ConnectionOptionsBaton* baton = new ConnectionOptionsBaton();
memset(baton, 0, sizeof(ConnectionOptionsBaton));

baton->fd = fd;
baton->baudRate = getIntFromObject(options, "baudRate");
Expand Down Expand Up @@ -169,7 +167,6 @@ NAN_METHOD(Close) {
}

VoidBaton* baton = new VoidBaton();
memset(baton, 0, sizeof(VoidBaton));
baton->fd = Nan::To<v8::Int32>(info[0]).ToLocalChecked()->Value();
baton->callback.Reset(info[1].As<v8::Function>());

Expand Down Expand Up @@ -210,7 +207,6 @@ NAN_METHOD(Flush) {
v8::Local<v8::Function> callback = info[1].As<v8::Function>();

VoidBaton* baton = new VoidBaton();
memset(baton, 0, sizeof(VoidBaton));
baton->fd = fd;
baton->callback.Reset(callback);

Expand Down Expand Up @@ -261,7 +257,6 @@ NAN_METHOD(Set) {
v8::Local<v8::Function> callback = info[2].As<v8::Function>();

SetBaton* baton = new SetBaton();
memset(baton, 0, sizeof(SetBaton));
baton->fd = fd;
baton->callback.Reset(callback);
baton->brk = getBoolFromObject(options, "brk");
Expand Down Expand Up @@ -308,7 +303,6 @@ NAN_METHOD(Get) {
}

GetBaton* baton = new GetBaton();
memset(baton, 0, sizeof(GetBaton));
baton->fd = fd;
baton->cts = false;
baton->dsr = false;
Expand Down Expand Up @@ -360,7 +354,6 @@ NAN_METHOD(Drain) {
}

VoidBaton* baton = new VoidBaton();
memset(baton, 0, sizeof(VoidBaton));
baton->fd = fd;
baton->callback.Reset(info[1].As<v8::Function>());

Expand Down
1 change: 0 additions & 1 deletion src/serialport_unix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,6 @@ int setup(int fd, OpenBaton *data) {

// Copy the connection options into the ConnectionOptionsBaton to set the baud rate
ConnectionOptionsBaton* connectionOptions = new ConnectionOptionsBaton();
memset(connectionOptions, 0, sizeof(ConnectionOptionsBaton));
connectionOptions->fd = fd;
connectionOptions->baudRate = data->baudRate;

Expand Down
10 changes: 6 additions & 4 deletions src/serialport_win.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,6 @@ NAN_METHOD(Write) {
}

WriteBaton* baton = new WriteBaton();
memset(baton, 0, sizeof(WriteBaton));
baton->fd = fd;
baton->buffer.Reset(buffer);
baton->bufferData = bufferData;
Expand Down Expand Up @@ -320,14 +319,14 @@ DWORD __stdcall WriteThread(LPVOID param) {
WriteBaton* baton = static_cast<WriteBaton*>(param);

OVERLAPPED* ov = new OVERLAPPED;
memset(ov, 0, sizeof(ov));
memset(ov, 0, sizeof(OVERLAPPED));
ov->hEvent = static_cast<void*>(baton);

while (!baton->complete) {
char* offsetPtr = baton->bufferData + baton->offset;
// WriteFileEx requires calling GetLastError even upon success. Clear the error beforehand.
SetLastError(0);
WriteFileEx((HANDLE)baton->fd, offsetPtr, static_cast<DWORD>(baton->bufferLength), ov, WriteIOCompletion);
WriteFileEx((HANDLE)baton->fd, offsetPtr, static_cast<DWORD>(baton->bufferLength - baton->offset), ov, WriteIOCompletion);
// Error codes when call is successful, such as ERROR_MORE_DATA.
DWORD lastError = GetLastError();
if (lastError != ERROR_SUCCESS) {
Expand All @@ -350,6 +349,7 @@ void EIO_AfterWrite(uv_async_t* req) {
Nan::HandleScope scope;
WriteBaton* baton = static_cast<WriteBaton*>(req->data);
WaitForSingleObject(baton->hThread, INFINITE);
CloseHandle(baton->hThread);
delete req;

v8::Local<v8::Value> argv[1];
Expand Down Expand Up @@ -404,7 +404,6 @@ NAN_METHOD(Read) {
}

ReadBaton* baton = new ReadBaton();
memset(baton, 0, sizeof(ReadBaton));
baton->fd = fd;
baton->offset = offset;
baton->bytesToRead = bytesToRead;
Expand Down Expand Up @@ -434,6 +433,7 @@ void __stdcall ReadIOCompletion(DWORD errorCode, DWORD bytesTransferred, OVERLAP
return;
}
if (bytesTransferred) {
baton->bytesToRead -= bytesTransferred;
baton->bytesRead += bytesTransferred;
baton->offset += bytesTransferred;
}
Expand All @@ -454,6 +454,7 @@ void __stdcall ReadIOCompletion(DWORD errorCode, DWORD bytesTransferred, OVERLAP
char* offsetPtr = baton->bufferData + baton->offset;

// ReadFile, unlike ReadFileEx, needs an event in the overlapped structure.
memset(ov, 0, sizeof(OVERLAPPED));
ov->hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
if (!ReadFile((HANDLE)baton->fd, offsetPtr, baton->bytesToRead, &bytesTransferred, ov)) {
errorCode = GetLastError();
Expand Down Expand Up @@ -526,6 +527,7 @@ void EIO_AfterRead(uv_async_t* req) {
Nan::HandleScope scope;
ReadBaton* baton = static_cast<ReadBaton*>(req->data);
WaitForSingleObject(baton->hThread, INFINITE);
CloseHandle(baton->hThread);
delete req;

v8::Local<v8::Value> argv[2];
Expand Down

0 comments on commit 0e4b1f9

Please sign in to comment.