Fix one (of many) build issues on Windows

fix typo in README.md

.github/workflows/ci.yml

@@ -17,13 +17,13 @@ jobs:
       fail-fast: false
       matrix:
         node: [12.14.1]
-        os: [macos-latest, windows-latest, ubuntu-latest]
+        os: [macos-10.14, windows-2019, ubuntu-18.04]
         include:
-          - os: macos-latest
+          - os: macos-10.14
             friendlyName: macOS
-          - os: windows-latest
+          - os: windows-2019
             friendlyName: Windows
-          - os: ubuntu-latest
+          - os: ubuntu-18.04
             friendlyName: Linux
     steps:
       - uses: actions/checkout@v2

README.md

@@ -9,7 +9,7 @@ The intention is to support the same platforms that
 
 ## Building
 
-This project is written as a Node project with the C-portions beign compiled by
+This project is written as a Node project with the C-portions being compiled by
 node-gyp. Installing dependencies and building requires Node.js, and yarn. With
 those prerequisites the initial setup should be as easy as running `yarn` and
 subsequent builds can be done using `yarn build`. There are some tests available

binding.gyp

@@ -13,12 +13,25 @@
         'xcode_settings': {
           'OTHER_CFLAGS': [
             '-Wall',
-            '-Werror'
-          ],
+            '-Werror',
+            '-Werror=format-security',
+            '-fPIC',
+            '-D_FORTIFY_SOURCE=1',
+            '-fstack-protector-strong'
+          ]
         },
         'cflags!': [
           '-Wall',
           '-Werror',
+          '-fPIC',
+          '-pie',
+          '-D_FORTIFY_SOURCE=1',
+          '-fstack-protector-strong',
+          '-Werror=format-security'
+        ],
+        'ldflags!': [
+          '-z relro',
+          '-z now'
         ],
         'conditions': [
           ['OS=="win"', {

docs/releases.md

@@ -0,0 +1,25 @@
+# Releases
+
+All releases are published using GitHub releases. Anyone with push access to the
+repository can create a new release.
+
+### Release Process
+
+1. Create a branch named `releases/X.Y.Z`, where `X.Y.Z` is the version you want
+   to release.
+1. Update the `version` field in the `package.json` with the new version you're
+   about to release.
+1. Open a Pull Request for that branch.
+1. Once the branch is approved, `git tag vX.Y.Z` the version you wish to
+   publish. **Important:** the version in the tag name must be preceeded by a
+   `v`.
+1. `git push --follow-tags` to ensure all new commits (and the tag) are pushed
+   to the remote. Pushing the tag will start the release process.
+1. Wait a few minutes for the build to finish (look for the build in
+   https://github.com/desktop/desktop-trampoline/actions)
+1. Once the build is complete it will create a new release with all of the
+   assets and suggested release notes.
+1. Update the changelog to whatever makes sense for this release. It should be
+   focused on user-facing changes.
+1. Confirm all assets are uploaded for all the supported platforms.
+1. Merge the Pull Request into `main` and you're done :tada:

package.json

@@ -1,6 +1,6 @@
 {
   "name": "desktop-trampoline",
-  "version": "0.9.0",
+  "version": "0.9.4",
   "main": "index.js",
   "keywords": [],
   "author": "",

src/desktop-trampoline.c

@@ -1,5 +1,7 @@
 #include <errno.h>
+#include <fcntl.h>
 #include <stdarg.h>
+#include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -17,10 +19,9 @@ if (writeSocket(socket, dataString, strlen(dataString) + 1) != 0) { \
 
 // This is a list of valid environment variables that GitHub Desktop might
 // send or expect to receive.
-#define NUMBER_OF_VALID_ENV_VARS 5
+#define NUMBER_OF_VALID_ENV_VARS 4
 static const char *sValidEnvVars[NUMBER_OF_VALID_ENV_VARS] = {
   "DESKTOP_TRAMPOLINE_IDENTIFIER",
-  "DESKTOP_PORT",
   "DESKTOP_TRAMPOLINE_TOKEN",
   "DESKTOP_USERNAME",
   "DESKTOP_ENDPOINT",
@@ -33,8 +34,8 @@ int isValidEnvVar(char *env) {
 
     // Make sure that not only the passed env var string starts with the
     // candidate contesnts, but also that there is a '=' character right after:
-    // Valid: "DESKTOP_PORT=50"
-    // Not valid: "DESKTOP_PORT_SOMETHING=50"
+    // Valid: "DESKTOP_USERNAME=sergiou87"
+    // Not valid: "DESKTOP_USERNAME_SOMETHING=sergiou87"
     if (strncmp(env, candidate, strlen(candidate)) == 0
         && strlen(env) > strlen(candidate)
         && env[strlen(candidate)] == '=') {
@@ -45,6 +46,42 @@ int isValidEnvVar(char *env) {
   return 0;
 }
 
+/**
+ * Reads a string from the socket, reading first 2 bytes to get its length and
+ * then the string itself.
+ */
+ssize_t readDelimitedString(SOCKET socket, char *buffer, size_t bufferSize) {
+  uint16_t outputLength = 0;
+  if (readSocket(socket, &outputLength, sizeof(uint16_t)) < (int)sizeof(uint16_t)) {
+      printSocketError("ERROR: Error reading from socket");
+      return -1;
+  }
+
+  if (outputLength > bufferSize) {
+    fprintf(stderr, "ERROR: received string is bigger than buffer (%d > %zu)", outputLength, bufferSize);
+    return -1;
+  }
+
+  size_t totalBytesRead = 0;
+  ssize_t bytesRead = 0;
+
+  // Read output from server
+  do {
+    bytesRead = readSocket(socket, buffer + totalBytesRead, outputLength - totalBytesRead);
+
+    if (bytesRead == -1) {
+      printSocketError("ERROR: Error reading from socket");
+      return -1;
+    }
+
+    totalBytesRead += bytesRead;
+  } while (bytesRead > 0);
+
+  buffer[totalBytesRead] = '\0';
+
+  return totalBytesRead;
+}
+
 int runTrampolineClient(SOCKET *outSocket, int argc, char **argv, char **envp) {
   char *desktopPortString;
 
@@ -67,7 +104,9 @@ int runTrampolineClient(SOCKET *outSocket, int argc, char **argv, char **envp) {
   *outSocket = socket;
 
   if (connectSocket(socket, desktopPort) != 0) {
-    printSocketError("ERROR: Couldn't connect to 127.0.0.1:%d", desktopPort);
+    printSocketError("ERROR: Couldn't connect to 127.0.0.1:%d - Please make "
+                     "sure you don't have an antivirus or firewall blocking "
+                     "this connection.", desktopPort);
     return 1;
   }
 
@@ -101,29 +140,57 @@ int runTrampolineClient(SOCKET *outSocket, int argc, char **argv, char **envp) {
     WRITE_STRING_OR_EXIT("environment variable", validEnvVars[idx]);
   }
 
-  // TODO: send stdin stuff?
+  char buffer[BUFFER_LENGTH + 1];
+  size_t totalBytesWritten = 0;
+  ssize_t bytesToWrite = 0;
 
-  char buffer[BUFFER_LENGTH];
-  size_t totalBytesRead = 0;
-  ssize_t bytesRead = 0;
+  // Make stdin reading non-blocking, to prevent getting stuck when no data is
+  // provided via stdin.
+  int flags = fcntl(STDIN_FILENO, F_GETFL, 0);
+  fcntl(STDIN_FILENO, F_SETFL, flags | O_NONBLOCK);
 
-  // Read output from server
+  // Send stdin data
   do {
-    bytesRead = readSocket(socket, buffer + totalBytesRead, BUFFER_LENGTH - totalBytesRead);
+    bytesToWrite = read(0, buffer, BUFFER_LENGTH);
 
-    if (bytesRead == -1) {
-      printSocketError("ERROR: Error reading from socket");
+    if (bytesToWrite == -1) {
+      if (totalBytesWritten == 0) {
+        // No stdin content found, continuing...
+        break;
+      } else {
+        fprintf(stderr, "ERROR: Error reading stdin data");
+        return 1;
+      }
+    }
+
+    if (writeSocket(socket, buffer, bytesToWrite) != 0) {
+      printSocketError("ERROR: Couldn't send stdin data");
       return 1;
     }
 
-    totalBytesRead += bytesRead;
-  } while (bytesRead > 0);
+    totalBytesWritten += bytesToWrite;
+  } while (bytesToWrite > 0);
 
-  buffer[totalBytesRead] = '\0';
+  writeSocket(socket, "\0", 1);
+
+  // Read stdout from the server
+  if (readDelimitedString(socket, buffer, BUFFER_LENGTH) == -1) {
+    fprintf(stderr, "ERROR: Couldn't read stdout from socket");
+    return 1;
+  }
 
   // Write that output to stdout
   fprintf(stdout, "%s", buffer);
 
+  // Read stderr from the server
+  if (readDelimitedString(socket, buffer, BUFFER_LENGTH) == -1) {
+    fprintf(stderr, "ERROR: Couldn't read stdout from socket");
+    return 1;
+  }
+
+  // Write that output to stderr
+  fprintf(stderr, "%s", buffer);
+
   return 0;
 }
 

src/socket.c

@@ -6,13 +6,30 @@
 #include <stdlib.h>
 #include <string.h>
 
+#ifdef WINDOWS
+
+#define MAX_WSA_ERROR_DESCRIPTION_LENGTH 4096
+
+void getWSALastErrorDescription(wchar_t *buffer, int bufferLength) {
+  FormatMessageW(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, 
+                 NULL, WSAGetLastError(),
+                 MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
+                 (LPWSTR)buffer, bufferLength - 1, NULL);
+}
+
+#endif
+
 int initializeNetwork(void) {
 #ifdef WINDOWS
   // Initialize Winsock
   WSADATA wsaData;
   int result = WSAStartup(MAKEWORD(2,2), &wsaData);
   if (result != NO_ERROR) {
-    fprintf(stderr, "ERROR: WSAStartup failed: %d\n", result);
+    wchar_t errorDescription[MAX_WSA_ERROR_DESCRIPTION_LENGTH];
+    getWSALastErrorDescription(errorDescription, MAX_WSA_ERROR_DESCRIPTION_LENGTH);
+
+    fprintf(stderr, "ERROR: WSAStartup failed (%d). Error %ld: %ls\n",
+            result, WSAGetLastError(), errorDescription);
     return 1;
   }
 #endif
@@ -55,8 +72,7 @@ int readSocket(SOCKET socket, void *buffer, size_t length) {
   return recv(socket, buffer, length, 0);
 }
 
-void printSocketError(char *fmt, ...) 
-{    
+void printSocketError(char *fmt, ...) {
   char formatted_string[4096];
 
   va_list argptr;
@@ -65,7 +81,10 @@ void printSocketError(char *fmt, ...)
   va_end(argptr);
 
 #ifdef WINDOWS
-  fprintf(stderr, "%s: %ld\n", formatted_string, WSAGetLastError());
+  wchar_t errorDescription[MAX_WSA_ERROR_DESCRIPTION_LENGTH];
+  getWSALastErrorDescription(errorDescription, MAX_WSA_ERROR_DESCRIPTION_LENGTH);
+
+  fprintf(stderr, "%s (%ld): %ls\n", formatted_string, WSAGetLastError(), errorDescription);
 #else
   fprintf(stderr, "%s (%d): %s\n", formatted_string, errno, strerror(errno));
 #endif

test/index.test.js

@@ -9,6 +9,44 @@ const { createServer } = require('net')
 const trampolinePath = getDesktopTrampolinePath()
 const run = promisify(execFile)
 
+async function startTrampolineServer(output, stdout = '', stderr = '') {
+  const server = createServer(socket => {
+    socket.pipe(split2(/\0/)).on('data', data => {
+      output.push(data.toString('utf8'))
+    })
+
+    const buffer = Buffer.alloc(2, 0)
+
+    // Send stdout
+    buffer.writeUInt16LE(stdout.length, 0)
+    socket.write(buffer)
+    socket.write(stdout)
+
+    // Send stderr
+    buffer.writeUInt16LE(stderr.length, 0)
+    socket.write(buffer)
+    socket.write(stderr)
+
+    // Close the socket
+    socket.end()
+  })
+  server.unref()
+
+  const startServer = async () => {
+    return new Promise((resolve, reject) => {
+      server.on('error', e => reject(e))
+      server.listen(0, '127.0.0.1', () => {
+        resolve(server.address().port)
+      })
+    })
+  }
+
+  return {
+    server,
+    port: await startServer(),
+  }
+}
+
 describe('desktop-trampoline', () => {
   it('exists and is a regular file', async () =>
     expect((await stat(trampolinePath)).isFile()).toBe(true))
@@ -19,46 +57,127 @@ describe('desktop-trampoline', () => {
   it('fails when required environment variables are missing', () =>
     expect(run(trampolinePath, ['Username'])).rejects.toThrow())
 
-  it('forwards arguments and valid environment variables correctly', async () => {
-    const output = []
-    const server = createServer(socket => {
-      socket.pipe(split2(/\0/)).on('data', data => {
-        output.push(data.toString('utf8'))
-      })
+  describe('with a trampoline server', () => {
+    let server = null
+    let output = []
+    let baseEnv = {}
 
-      // Don't send anything and just close the socket after the trampoline is
-      // done forwarding data.
-      socket.end()
+    async function configureTrampolineServer(stdout = '', stderr = '') {
+      output = []
+      const serverInfo = await startTrampolineServer(output, stdout, stderr)
+      server = serverInfo.server
+      baseEnv = {
+        DESKTOP_PORT: serverInfo.port,
+      }
+    }
+
+    afterEach(() => {
+      server.close()
     })
 
-    const startTrampolineServer = async () => {
-      return new Promise((resolve, reject) => {
-        server.on('error', e => reject(e))
-        server.listen(0, '127.0.0.1', () => {
-          resolve(server.address().port)
+    it('forwards arguments and valid environment variables correctly without stdin', async () => {
+      await configureTrampolineServer()
+
+      const env = {
+        ...baseEnv,
+        DESKTOP_TRAMPOLINE_IDENTIFIER: '123456',
+        DESKTOP_USERNAME: 'sergiou87',
+        DESKTOP_USERNAME_FAKE: 'fake-user',
+        INVALID_VARIABLE: 'foo bar',
+      }
+      const opts = { env }
+
+      await run(trampolinePath, ['baz'], opts)
+
+      const outputArguments = output.slice(1, 2)
+      expect(outputArguments).toStrictEqual(['baz'])
+      // output[2] is the number of env variables
+      const outputEnv = output.slice(3, output.length - 1)
+      expect(outputEnv).toHaveLength(2)
+      expect(outputEnv).toContain('DESKTOP_TRAMPOLINE_IDENTIFIER=123456')
+      expect(outputEnv).toContain(`DESKTOP_USERNAME=sergiou87`)
+
+      expect(output[output.length - 1]).toStrictEqual('')
+    })
+
+    it('forwards arguments, environment variables and stdin correctly', async () => {
+      await configureTrampolineServer()
+
+      const env = {
+        ...baseEnv,
+        DESKTOP_TRAMPOLINE_IDENTIFIER: '123456',
+        DESKTOP_USERNAME: 'sergiou87',
+      }
+      const opts = {
+        env,
+        stdin: 'This is a test\nWith a multiline\nStandard input text',
+      }
+
+      const run = new Promise((resolve, reject) => {
+        const process = execFile(trampolinePath, ['baz'], opts, function (
+          err,
+          stdout,
+          stderr
+        ) {
+          if (!err) {
+            resolve({ stdout, stderr, exitCode: 0 })
+            return
+          }
+
+          reject(err)
         })
+
+        process.stdin.end(
+          'This is a test\nWith a multiline\nStandard input text',
+          'utf-8'
+        )
       })
-    }
+      await run
 
-    const port = await startTrampolineServer()
-    const env = {
-      DESKTOP_PORT_FAKE: 32123,
-      DESKTOP_TRAMPOLINE_IDENTIFIER: '123456',
-      DESKTOP_PORT: port,
-      INVALID_VARIABLE: 'foo bar',
-    }
-    const opts = { env }
+      const outputArguments = output.slice(1, 2)
+      expect(outputArguments).toStrictEqual(['baz'])
+      // output[2] is the number of env variables
+      const outputEnv = output.slice(3, output.length - 1)
+      expect(outputEnv).toHaveLength(2)
+      expect(outputEnv).toContain('DESKTOP_TRAMPOLINE_IDENTIFIER=123456')
+      expect(outputEnv).toContain(`DESKTOP_USERNAME=sergiou87`)
 
-    await run(trampolinePath, ['baz'], opts)
+      expect(output[output.length - 1]).toBe(
+        'This is a test\nWith a multiline\nStandard input text'
+      )
+    })
 
-    const outputArguments = output.slice(1, 2)
-    expect(outputArguments).toStrictEqual(['baz'])
-    // output[2] is the number of env variables
-    const outputEnv = output.slice(3)
-    expect(outputEnv).toHaveLength(2)
-    expect(outputEnv).toContain('DESKTOP_TRAMPOLINE_IDENTIFIER=123456')
-    expect(outputEnv).toContain(`DESKTOP_PORT=${port}`)
+    it('outputs the same stdout received from the server, when no stderr is specified', async () => {
+      await configureTrampolineServer('This is the command stdout', '')
 
-    server.close()
+      const opts = { env: baseEnv }
+      const result = await run(trampolinePath, ['baz'], opts)
+
+      expect(result.stdout).toStrictEqual('This is the command stdout')
+      expect(result.stderr).toStrictEqual('')
+    })
+
+    it('outputs the same stderr received from the server, when no stdout is specified', async () => {
+      await configureTrampolineServer('', 'This is the command stderr')
+
+      const opts = { env: baseEnv }
+      const result = await run(trampolinePath, ['baz'], opts)
+
+      expect(result.stdout).toStrictEqual('')
+      expect(result.stderr).toStrictEqual('This is the command stderr')
+    })
+
+    it('outputs the same stdout and stderr received from the server, when both are specified', async () => {
+      await configureTrampolineServer(
+        'This is the command stdout',
+        'This is the command stderr'
+      )
+
+      const opts = { env: baseEnv }
+      const result = await run(trampolinePath, ['baz'], opts)
+
+      expect(result.stdout).toStrictEqual('This is the command stdout')
+      expect(result.stderr).toStrictEqual('This is the command stderr')
+    })
   })
 })