[488460] unchecked error return codes

[epakai]

BTS_msg_id: 20080629033848.GQ21122@outflux.net BTS author: Kees Cook kees@outflux.net

--HcccYpVZDxQ8hzPO Content-Type: text/plain; charset=us-ascii Content-Disposition: inline

Package: synergy Version: 1.3.1-4 Severity: normal Tags: patch User: ubuntu-devel@lists.ubuntu.com Usertags: origin-ubuntu intrepid ubuntu-patch

Hello,

This patch fixes a number of cases where error conditions are untested, which cause problems when compiling with -D_FORTIFY_SOURCE=2.

Thanks,

-Kees

-- Kees Cook @outflux.net

--HcccYpVZDxQ8hzPO Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="synergy-fortify.patch"

diff -u synergy-1.3.1/lib/platform/CXWindowsEventQueueBuffer.cpp synergy-1.3.1/lib/platform/CXWindowsEventQueueBuffer.cpp --- synergy-1.3.1/lib/platform/CXWindowsEventQueueBuffer.cpp +++ synergy-1.3.1/lib/platform/CXWindowsEventQueueBuffer.cpp @@ -82,7 +82,7 @@ // clear out the pipe in preparation for waiting.

char buf[16];

• read(m_pipefd[0], buf, 15);

• if (read(m_pipefd[0], buf, 15)) {};

  { CLock lock(&m_mutex); @@ -141,7 +141,7 @@

  if HAVE_POLL

  poll(pfds, 2, timeout); if (pfds[1].revents & POLLIN) {

• read(m_pipefd[0], buf, 15);
• if (read(m_pipefd[0], buf, 15)) {}; }

  else

  select(nfds, @@ -212,7 +212,7 @@ // that is waiting for a ConnectionNumber() socket to be readable. // The flush call can read incoming data from the socket and put // it in Xlib's input buffer. That sneaks it past the other thread.

• write(m_pipefd[1], "!", 1);

• if (write(m_pipefd[1], "!", 1)) {}; }

  return true; diff -u synergy-1.3.1/lib/arch/CArchDaemonUnix.cpp synergy-1.3.1/lib/arch/CArchDaemonUnix.cpp --- synergy-1.3.1/lib/arch/CArchDaemonUnix.cpp +++ synergy-1.3.1/lib/arch/CArchDaemonUnix.cpp @@ -58,7 +58,9 @@ setsid();

  // chdir to root so we don't keep mounted filesystems points busy

• chdir("/");
• if (chdir("/") < 0) {
• throw XArchDaemonFailed(new XArchEvalUnix(errno));

• }

  // mask off permissions for any but owner umask(077); @@ -70,9 +72,11 @@

  // attach file descriptors 0, 1, 2 to /dev/null so inadvertent use // of standard I/O safely goes in the bit bucket.

• open("/dev/null", O_RDONLY);
• open("/dev/null", O_RDWR);
• dup(1);
• if (open("/dev/null", O_RDONLY)<0 ||
• open("/dev/null", O_RDWR)<0 ||
• dup(1)<0) {
• throw XArchDaemonFailed(new XArchEvalUnix(errno));

• }

  // invoke function return func(1, &name); --- synergy-1.3.1.orig/lib/arch/CArchNetworkBSD.cpp +++ synergy-1.3.1/lib/arch/CArchNetworkBSD.cpp @@ -314,9 +314,11 @@ if (n > 0 && unblockPipe != NULL && (pfd[num].revents & POLLIN) != 0) { // the unblock event was signalled. flush the pipe. char dummy[100];

• do {
• read(unblockPipe[0], dummy, sizeof(dummy));
• } while (errno != EAGAIN);
• for (;;) {
• if (read(unblockPipe[0], dummy, sizeof(dummy)) < 0 &&
• errno != EAGAIN)
• break;

• }

  // don't count this unblock pipe in return value
  --n;

  @@ -487,7 +489,8 @@ const int* unblockPipe = getUnblockPipeForThread(thread); if (unblockPipe != NULL) { char dummy = 0;

• write(unblockPipe[1], &dummy, 1);
• if (write(unblockPipe[1], &dummy, 1)<0)
• throwError(errno); } }

--HcccYpVZDxQ8hzPO--

[epakai]

BTS_msg_id: 487B6E19.8030609@licquia.org BTS author: Jeff Licquia jeff@licquia.org

Kees Cook wrote:

This patch fixes a number of cases where error conditions are untested, which cause problems when compiling with -D_FORTIFY_SOURCE=2.

Hi! Sorry for the late reply; just got back from vacation.

I'm not super-keen on diverging from upstream in general unless absolutely necessary. Most of the changes in the patch are "tool patches"; they don't really fix problems in the code itself, but just issues with Fortify. I'm not inclined to include them unless they come down from upstream.

The only two parts that fix real issues, as I see it, are:

• the daemonization code

• the pipe flush (read) in lib/arch/CArchNetworkBSD.cpp

The daemonization code is just ugly. I smell a future bug in the code that relies on sequential file descriptor assignment by the OS. So that needs more work.

The pipe flush code is probably a little more correct, but I'd also like to see better handling of unexpected error conditions.