fix(windows): Fix read & write bugs for windows (#1364)

* Fix OVERLAPPED initialization * Fix buffer overrun for writing and reading * Remove unsafe memset for non-POD structures * Fix thread handle leak
serialport · Oct 16, 2017 · 0e4b1f9 · 0e4b1f9
1 parent 6ed7b3b
commit 0e4b1f9
Show file tree

Hide file tree

Showing 3 changed files with 6 additions and 12 deletions.
diff --git a/src/serialport.cpp b/src/serialport.cpp
@@ -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");
@@ -125,7 +124,6 @@ NAN_METHOD(Update) {
   }
 
   ConnectionOptionsBaton* baton = new ConnectionOptionsBaton();
-  memset(baton, 0, sizeof(ConnectionOptionsBaton));
 
   baton->fd = fd;
   baton->baudRate = getIntFromObject(options, "baudRate");
@@ -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>());
 
@@ -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);
 
@@ -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");
@@ -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;
@@ -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>());
 

diff --git a/src/serialport_unix.cpp b/src/serialport_unix.cpp
@@ -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;
 

diff --git a/src/serialport_win.cpp b/src/serialport_win.cpp
@@ -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;
@@ -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) {
@@ -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];
@@ -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;
@@ -434,6 +433,7 @@ void __stdcall ReadIOCompletion(DWORD errorCode, DWORD bytesTransferred, OVERLAP
     return;
   }
   if (bytesTransferred) {
+    baton->bytesToRead -= bytesTransferred;
     baton->bytesRead += bytesTransferred;
     baton->offset += bytesTransferred;
   }
@@ -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();
@@ -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];