Bug 10036 - Lack of Sanity Checking in calls to ftell()/fcntl()
Summary: Lack of Sanity Checking in calls to ftell()/fcntl()
Status: NEW
Alias: None
Product: Samba 4.0
Classification: Unclassified
Component: Other (show other bugs)
Version: 4.0.7
Hardware: All All
: P5 major (vote)
Target Milestone: ---
Assignee: Andrew Bartlett
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-18 17:19 UTC by Bill Parker
Modified: 2013-07-18 17:23 UTC (History)
0 users

See Also:


Attachments
Adds Sanity Check for ftell() call (388 bytes, application/octet-stream)
2013-07-18 17:19 UTC, Bill Parker
no flags Details
Adds Sanity Check for fcntl() call (706 bytes, patch)
2013-07-18 17:20 UTC, Bill Parker
wp02855: review?
Details
Adds Sanity Check for fcntl() call (402 bytes, patch)
2013-07-18 17:22 UTC, Bill Parker
wp02855: review?
Details
Adds Sanity Check for fcntl() call (607 bytes, patch)
2013-07-18 17:22 UTC, Bill Parker
wp02855: review?
Details
Adds Sanity Check for fcntl() call (425 bytes, patch)
2013-07-18 17:23 UTC, Bill Parker
wp02855: review?
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Bill Parker 2013-07-18 17:19:34 UTC
Created attachment 9060 [details]
Adds Sanity Check for ftell() call

Hello All,

	In reviewing various files in Samba-4.0.7, I found a number
of instances where ftell() and/or fcntl() were called without
the checking the return value for a value of -1, which would
indicate failure.

In directory 'samba-4.0.7/source3/passdb', file 'pdb_smbpasswd.c',
the following patch file makes the required check, though in some
cases, a(n) error message to stderr might be appropriate:

--- pdb_smbpasswd.c.orig        2013-07-17 19:04:25.962920605 -0700
+++ pdb_smbpasswd.c     2013-07-17 19:07:37.747919091 -0700
@@ -790,6 +790,11 @@
        status = linebuf;
        while (status && !feof(fp)) {
                pwd_seekpos = ftell(fp);
+               if (pwd_seekpos == -1) {
+                       DEBUG(0, ("mod_smbfilepwd_entry: unable to reposition file%\n", pfile));
+                       fclose(fp);
+                       return False;
+               }
 
                linebuf[0] = '\0';
 
	In reviewing various files in Samba-4.0.7, I found a number
of instances where fcntl() were called without the checking the
return value for a value of -1, which would indicate failure.

In directory 'samba-4.0.7/lib/async_req', file 'async_sock.c' the
following patch file makes the required check, though in some cases,
a(n) error message to stderr might be appropriate:

--- async_sock.c.orig   2013-07-17 19:22:43.493922674 -0700
+++ async_sock.c        2013-07-17 19:29:06.393912269 -0700
@@ -306,7 +306,10 @@
  post_errno:
        tevent_req_error(result, state->sys_errno);
  done:
-       fcntl(fd, F_SETFL, state->old_sockflags);
+       if (fcntl(fd, F_SETFL, state->old_sockflags) == -1) {
+               state->sys_errno = errno;
+               tevent_req_error(result, state->sys_errno);
+       }
        return tevent_req_post(result, ev);
 }
 
@@ -352,7 +355,9 @@
                tevent_req_data(req, struct async_connect_state);
        int err;
 
-       fcntl(state->fd, F_SETFL, state->old_sockflags);
+       if (fcntl(state->fd, F_SETFL, state->old_sockflags) == -1) {
+               return -1;
+       }
 
        if (tevent_req_is_unix_error(req, &err)) {
                *perrno = err;
				

In directory 'samba-4.0.7/source3/lib', file 'recvfile.c' the
following patch file makes the required check, though in some cases,
a(n) error message to stderr might be appropriate:

--- recvfile.c.orig     2013-07-17 19:50:43.587919082 -0700
+++ recvfile.c  2013-07-17 19:51:51.016918916 -0700
@@ -260,6 +260,11 @@
        }
 
        old_flags = fcntl(sockfd, F_GETFL, 0);
+       if (old_flags == -1) {
+               free(buffer);
+               return -1;
+       }
+
        if (set_blocking(sockfd, true) == -1) {
                free(buffer);
                return -1;
				
In directory 'samba-4.0.7/source3/smbd', file 'vfs.c' the
following patch file makes the required check, though in
some cases, a(n) error message to stderr might be appropriate:

--- vfs.c.orig  2013-07-17 19:56:28.243919876 -0700
+++ vfs.c       2013-07-17 19:57:58.805916306 -0700
@@ -436,6 +436,9 @@
                req->unread_bytes = 0;
                /* Ensure the socket is blocking. */
                old_flags = fcntl(sockfd, F_GETFL, 0);
+               if (old_flags == -1) {
+                       return (ssize_t)-1;
+               }
                if (set_blocking(sockfd, true) == -1) {
                        return (ssize_t)-1;
                }
@@ -480,6 +483,9 @@
                req->unread_bytes = 0;
                /* Ensure the socket is blocking. */
                old_flags = fcntl(sockfd, F_GETFL, 0);
+               if (old_flags == -1) {
+                       return (ssize_t)-1;
+               }
                if (set_blocking(sockfd, true) == -1) {
                        return (ssize_t)-1;
                }
				

In directory 'samba-4.0.7/tests', file 'fcntl_lock_thread.c'
the following patch file makes the required check, though in
some cases, a(n) error message to stderr might be appropriate:

--- fcntl_lock_thread.c.orig    2013-07-17 20:04:39.181918422 -0700
+++ fcntl_lock_thread.c 2013-07-17 20:07:17.767919133 -0700
@@ -97,7 +97,10 @@
        lock.l_pid = getpid();
 
        /* set a 4 byte write lock */
-       fcntl(fd,F_SETLK,&lock);
+       ret = fcntl(fd,F_SETLK,&lock);
+       if (ret == -1) {
+               fprintf(stderr,"ERROR: lock test failed (ret=%d errno=%d)\n", ret, (int)errno);
+       }
 
        sleep(4);  /* allow thread to try getting lock */

		
I am attaching these patches to the bug report

Bill Parker (wp02855 at gmail dot com)
Comment 1 Bill Parker 2013-07-18 17:20:22 UTC
Created attachment 9061 [details]
Adds Sanity Check for fcntl() call
Comment 2 Bill Parker 2013-07-18 17:22:02 UTC
Created attachment 9062 [details]
Adds Sanity Check for fcntl() call
Comment 3 Bill Parker 2013-07-18 17:22:49 UTC
Created attachment 9063 [details]
Adds Sanity Check for fcntl() call
Comment 4 Bill Parker 2013-07-18 17:23:16 UTC
Created attachment 9064 [details]
Adds Sanity Check for fcntl() call