Flag checking the bad way (+patch)

[Tilo Eckert <tilo.eckert@xxxxxxx>]

Hi,

I compiled libssh on a non-Linux system where all the O_ and S_ file
flags are defined differently. The way libssh checks these flags results
in some unexpected behavior. File access modes are defined as:
  #define O_WRONLY     0x01
  #define O_RDONLY     0x02
  #define O_RDWR       0x03

The way mode flags are checked in sftp_open resulted in read-only and
write-only being translated to read-write. For flags which may have more
than one bit set, ANDing them with the variable and checking for
non-zero is not sufficient. Instead, the following pattern must be used
(which is always a good idea to use to avoid such platform-dependency
issues):
  if (((flags & FLAG) == FLAG))

Since the S_IF flags values are different too, sftp_parse_attr functions
failed to translate the types of files received by sftp_readdir.
Replacing all S_IF flags with own defines for platform-independence is a
good idea.

Patches for both are attached. Note that I only checked the SFTP module
for flags issues. Someone might want to look for other similar places.

Tilo

From 1a0b6de1b7e544b123132343e59e79028a4b7391 Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Wed, 22 Jul 2015 15:26:56 +0200
Subject: fix file mode checks in sftp_open()

Signed-off-by: Tilo Eckert <tilo.eckert@xxxxxxx>
---
 src/sftp.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/src/sftp.c b/src/sftp.c
index e925b52..ae7b7ef 100644
--- a/src/sftp.c
+++ b/src/sftp.c
@@ -1638,18 +1638,17 @@ sftp_file sftp_open(sftp_session sftp, const char *file, int flags,
   attr.permissions = mode;
   attr.flags = SSH_FILEXFER_ATTR_PERMISSIONS;
 
-  if (flags == O_RDONLY)
-    sftp_flags |= SSH_FXF_READ; /* if any of the other flag is set,
-                                   READ should not be set initialy */
-  if (flags & O_WRONLY)
+  if ((flags & O_RDONLY) == O_RDONLY)
+    sftp_flags |= SSH_FXF_READ;
+  if ((flags & O_WRONLY) == O_WRONLY)
     sftp_flags |= SSH_FXF_WRITE;
-  if (flags & O_RDWR)
+  if ((flags & O_RDWR) == O_RDWR)
     sftp_flags |= (SSH_FXF_WRITE | SSH_FXF_READ);
-  if (flags & O_CREAT)
+  if ((flags & O_CREAT) == O_CREAT)
     sftp_flags |= SSH_FXF_CREAT;
-  if (flags & O_TRUNC)
+  if ((flags & O_TRUNC) == O_TRUNC)
     sftp_flags |= SSH_FXF_TRUNC;
-  if (flags & O_EXCL)
+  if ((flags & O_EXCL) == O_EXCL)
     sftp_flags |= SSH_FXF_EXCL;
   SSH_LOG(SSH_LOG_PACKET,"Opening file %s with sftp flags %x",file,sftp_flags);
   id = sftp_get_new_id(sftp);
-- 
2.4.5

From ce313999b6fd4a85b7ed67ea0b3cf55ad6e676f0 Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Wed, 22 Jul 2015 15:24:14 +0200
Subject: define our own platform-independent S_IF macros

Signed-off-by: Tilo Eckert <tilo.eckert@xxxxxxx>
---
 include/libssh/sftp.h | 10 ++++++++++
 src/sftp.c            | 40 ++++++++++++++++------------------------
 2 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/include/libssh/sftp.h b/include/libssh/sftp.h
index 8fb8f11..6620841 100644
--- a/include/libssh/sftp.h
+++ b/include/libssh/sftp.h
@@ -962,6 +962,16 @@ void sftp_handle_remove(sftp_session sftp, void *handle);
 #define SSH_FXF_EXCL 0x20
 #define SSH_FXF_TEXT 0x40
 
+/* file type flags */
+#define SSH_S_IFMT   00170000
+#define SSH_S_IFSOCK 0140000
+#define SSH_S_IFLNK  0120000
+#define SSH_S_IFREG  0100000
+#define SSH_S_IFBLK  0060000
+#define SSH_S_IFDIR  0040000
+#define SSH_S_IFCHR  0020000
+#define SSH_S_IFIFO  0010000
+
 /* rename flags */
 #define SSH_FXF_RENAME_OVERWRITE  0x00000001
 #define SSH_FXF_RENAME_ATOMIC     0x00000002
diff --git a/src/sftp.c b/src/sftp.c
index ae7b7ef..56093fb 100644
--- a/src/sftp.c
+++ b/src/sftp.c
@@ -38,14 +38,6 @@
 #ifndef _WIN32
 #include <netinet/in.h>
 #include <arpa/inet.h>
-#else
-#define S_IFSOCK 0140000
-#define S_IFLNK  0120000
-
-#ifdef _MSC_VER
-#define S_IFBLK  0060000
-#define S_IFIFO  0010000
-#endif
 #endif
 
 #include "libssh/priv.h"
@@ -1008,20 +1000,20 @@ static sftp_attributes sftp_parse_attr_4(sftp_session sftp, ssh_buffer buf,
       attr->permissions = ntohl(attr->permissions);
 
       /* FIXME on windows! */
-      switch (attr->permissions & S_IFMT) {
-        case S_IFSOCK:
-        case S_IFBLK:
-        case S_IFCHR:
-        case S_IFIFO:
+      switch (attr->permissions & SSH_S_IFMT) {
+        case SSH_S_IFSOCK:
+        case SSH_S_IFBLK:
+        case SSH_S_IFCHR:
+        case SSH_S_IFIFO:
           attr->type = SSH_FILEXFER_TYPE_SPECIAL;
           break;
-        case S_IFLNK:
+        case SSH_S_IFLNK:
           attr->type = SSH_FILEXFER_TYPE_SYMLINK;
           break;
-        case S_IFREG:
+        case SSH_S_IFREG:
           attr->type = SSH_FILEXFER_TYPE_REGULAR;
           break;
-        case S_IFDIR:
+        case SSH_S_IFDIR:
           attr->type = SSH_FILEXFER_TYPE_DIRECTORY;
           break;
         default:
@@ -1244,20 +1236,20 @@ static sftp_attributes sftp_parse_attr_3(sftp_session sftp, ssh_buffer buf,
             goto error;
         }
 
-        switch (attr->permissions & S_IFMT) {
-        case S_IFSOCK:
-        case S_IFBLK:
-        case S_IFCHR:
-        case S_IFIFO:
+        switch (attr->permissions & SSH_S_IFMT) {
+        case SSH_S_IFSOCK:
+        case SSH_S_IFBLK:
+        case SSH_S_IFCHR:
+        case SSH_S_IFIFO:
             attr->type = SSH_FILEXFER_TYPE_SPECIAL;
             break;
-        case S_IFLNK:
+        case SSH_S_IFLNK:
             attr->type = SSH_FILEXFER_TYPE_SYMLINK;
             break;
-        case S_IFREG:
+        case SSH_S_IFREG:
             attr->type = SSH_FILEXFER_TYPE_REGULAR;
             break;
-        case S_IFDIR:
+        case SSH_S_IFDIR:
             attr->type = SSH_FILEXFER_TYPE_DIRECTORY;
             break;
         default:
-- 
2.4.5