From df107149913f82e8b3b6b2060d6dbed3d90e68fe Mon Sep 17 00:00:00 2001
From: Phlosioneer <mattmdrr2@gmail.com>
Date: Sun, 19 Nov 2017 19:55:28 -0500
Subject: server.c: Fix bug in release mode (#7594)

When compiling with CMAKE_BUILD_TYPE=RelWithDebInfo, several
-Wmaybe-uninitialized warnings are printed. These were thought to
be false positives (#5061); there are no control paths that lead
to an uninitialized value. However, when gcc is run in -O2 mode,
it makes a mistake while generating the necessary logic.

Specifically, for the code:
...
  int = 0; // Index of the server whose address equals addr.
  for (; i < watchers.ga_len; i++) {
    watcher = ((SocketWatcher **)watchers.ga_data)[i];
    // <snip>
  }
  if (i >= watchers.ga_len) {
    ELOG("Not listening on %s", addr);
    return;
  }
...

Gcc generates:
...
<+98>:  cmp  %ebx, %ebp
<+100>: jg   0x530f13   <server_stop+55>
<+102>: cmp  %ebp, ebx
<+104>: jl   0x530f7e   <server_stop+162>
...

Normally, the if statement should catch the only control path
where watcher is not assigned: watchers.ga_len <= 0. When
compiled, the assembly lines 98 and 100 correspond to checking if
i < watchers.ga_len, and the lines 102 and 104 correspond to
checking if i >= watchers.ga_len. The assembly seems to compare
ebp (which is watchers.ga_len) with ebx (which is i), and jump
if greater; then do the same comparison and jump if less. This is
where gcc makes a mistake: it flips the order of the cmp
instruction. This means that the REAL behavior is first check if
i < watchers.ga_len and then check if i < watchers.ga_len. Which
means the code inside the if statement is NEVER executed; no
combination of i and watchers.ga_len will ever trigger ELOG().

So not only is this a use of an uninitialized value if
watchers.ga_len == 0 (or technically, if it's less than zero too),
it also clobbers any error detection if the for loop reaches the
last entry (which would normally cause i == watchers.ga_len too).

This commit fixes this issue by adding a bool to keep track of
whether a watcher was found during the loop. This makes gcc
generate the correct code, avoiding both bugs.
---
 src/nvim/msgpack_rpc/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

(limited to 'src')

diff --git a/src/nvim/msgpack_rpc/server.c b/src/nvim/msgpack_rpc/server.c
index 1e0cc27886..9bf122f4db 100644
--- a/src/nvim/msgpack_rpc/server.c
+++ b/src/nvim/msgpack_rpc/server.c
@@ -180,6 +180,7 @@ int server_start(const char *endpoint)
 void server_stop(char *endpoint)
 {
   SocketWatcher *watcher;
+  bool watcher_found = false;
   char addr[ADDRESS_MAX_SIZE];
 
   // Trim to `ADDRESS_MAX_SIZE`
@@ -189,11 +190,12 @@ void server_stop(char *endpoint)
   for (; i < watchers.ga_len; i++) {
     watcher = ((SocketWatcher **)watchers.ga_data)[i];
     if (strcmp(addr, watcher->addr) == 0) {
+      watcher_found = true;
       break;
     }
   }
 
-  if (i >= watchers.ga_len) {
+  if (!watcher_found) {
     ELOG("Not listening on %s", addr);
     return;
   }
-- 
cgit