In reviewing the net/socket.c code I found a couple of resource leaks and added
some safety check messages to look for problems.
In the sockfd_lookup() routine I've added a test to ensure that the file matches
the value in sock->file, since the latter is used to release the use count.
In sock_release() I added a test to ensure that the fasync list is empty when
the socket is being released.
In the sys_socketpair routine, there was a possible resource leak in that an
error getting the second socket would exit without releasing the use count on
the first socket. I've streamlined the error cleanup to fix the problems and
minimize duplicated code.
In the sys_accept routine, a restart to handle a race condition would add
another use count to the first socket, causing a resource leak. I've fixed this
by moving the restart point after the first socket has been looked up. I think
this is the correct fix, but maybe someone else could look it over too?
The patch also replaces references to current->files->fd[xx] with sock->file
after a sockfd_lookup has succeeded. This is both shorter and safer, as it's
possible that the fd could have been released and reassigned after the
sockfd_lookup. (And I want to get rid of all the current->files->fd[] references
anyway.)
I'm running the patch with no problems or error messages so far, but would
appreciate a review of the changes.
Regards,
Bill
--------------7FDB60396CD3222E7DE77675
Content-Type: text/plain; charset=us-ascii; name="socket_88-patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="socket_88-patch"
--- linux-2.1.88/net/socket.c.old Tue Jan 27 09:36:37 1998
+++ linux-2.1.88/net/socket.c Thu Mar 5 12:25:32 1998
@@ -221,7 +221,7 @@
*/
inode->i_count++;
- current->files->fd[fd] = file;
+ fd_install(fd, file);
file->f_op = &socket_file_ops;
file->f_mode = 3;
file->f_flags = O_RDWR;
@@ -239,10 +239,11 @@
* Go from a file number to its socket slot.
*/
-extern __inline__ struct socket *sockfd_lookup(int fd, int *err)
+extern struct socket *sockfd_lookup(int fd, int *err)
{
struct file *file;
struct inode *inode;
+ struct socket *sock;
if (!(file = fget(fd)))
{
@@ -251,14 +252,18 @@
}
inode = file->f_dentry->d_inode;
- if (!inode || !inode->i_sock || !socki_lookup(inode))
+ if (!inode || !inode->i_sock || !(sock = socki_lookup(inode)))
{
*err = -ENOTSOCK;
fput(file);
return NULL;
}
- return socki_lookup(inode);
+ if (sock->file != file) {
+ printk(KERN_ERR "socki_lookup: socket file changed!\n");
+ sock->file = file;
+ }
+ return sock;
}
extern __inline__ void sockfd_put(struct socket *sock)
@@ -301,14 +306,15 @@
void sock_release(struct socket *sock)
{
- int oldstate;
-
- if ((oldstate = sock->state) != SS_UNCONNECTED)
+ if (sock->state != SS_UNCONNECTED)
sock->state = SS_DISCONNECTING;
if (sock->ops)
sock->ops->release(sock, NULL);
+ if (sock->fasync_list)
+ printk(KERN_ERR "sock_release: fasync list not empty!\n");
+
--sockets_in_use; /* Bookkeeping.. */
sock->file=NULL;
iput(sock->inode);
@@ -320,13 +326,10 @@
struct scm_cookie scm;
err = scm_send(sock, msg, &scm);
- if (err < 0)
- return err;
-
- err = sock->ops->sendmsg(sock, msg, size, &scm);
-
- scm_destroy(&scm);
-
+ if (err >= 0) {
+ err = sock->ops->sendmsg(sock, msg, size, &scm);
+ scm_destroy(&scm);
+ }
return err;
}
@@ -337,11 +340,8 @@
memset(&scm, 0, sizeof(scm));
size = sock->ops->recvmsg(sock, msg, size, flags, &scm);
-
- if (size < 0)
- return size;
-
- scm_recv(sock, msg, &scm, flags);
+ if (size >= 0)
+ scm_recv(sock, msg, &scm, flags);
return size;
}
@@ -663,9 +663,8 @@
asmlinkage int sys_socketpair(int family, int type, int protocol, int usockvec[2])
{
- int fd1, fd2, i;
- struct socket *sock1=NULL, *sock2=NULL;
- int err;
+ struct socket *sock1, *sock2;
+ int fd1, fd2, err;
lock_kernel();
@@ -674,48 +673,51 @@
* supports the socketpair call.
*/
- if ((fd1 = sys_socket(family, type, protocol)) < 0) {
- err = fd1;
+ err = sys_socket(family, type, protocol);
+ if (err < 0)
goto out;
- }
+ fd1 = err;
- sock1 = sockfd_lookup(fd1, &err);
- if (!sock1)
- goto out;
/*
- * Now grab another socket and try to connect the two together.
+ * Now grab another socket
*/
err = -EINVAL;
- if ((fd2 = sys_socket(family, type, protocol)) < 0)
- {
- sys_close(fd1);
- goto out;
- }
+ fd2 = sys_socket(family, type, protocol);
+ if (fd2 < 0)
+ goto out_close1;
- sock2 = sockfd_lookup(fd2,&err);
+ /*
+ * Get the sockets for the two fd's
+ */
+ sock1 = sockfd_lookup(fd1, &err);
+ if (!sock1)
+ goto out_close2;
+ sock2 = sockfd_lookup(fd2, &err);
if (!sock2)
- goto out;
- if ((i = sock1->ops->socketpair(sock1, sock2)) < 0)
- {
- sys_close(fd1);
+ goto out_put1;
+
+ /* try to connect the two sockets together */
+ err = sock1->ops->socketpair(sock1, sock2);
+ if (err < 0)
+ goto out_put2;
+
+ err = put_user(fd1, &usockvec[0]);
+ if (err)
+ goto out_put2;
+ err = put_user(fd2, &usockvec[1]);
+
+out_put2:
+ sockfd_put(sock2);
+out_put1:
+ sockfd_put(sock1);
+
+ if (err) {
+ out_close2:
sys_close(fd2);
- err = i;
- }
- else
- {
- err = put_user(fd1, &usockvec[0]);
- if (!err)
- err = put_user(fd2, &usockvec[1]);
- if (err) {
- sys_close(fd1);
- sys_close(fd2);
- }
+ out_close1:
+ sys_close(fd1);
}
out:
- if(sock1)
- sockfd_put(sock1);
- if(sock2)
- sockfd_put(sock2);
unlock_kernel();
return err;
}
@@ -790,58 +792,54 @@
int len;
lock_kernel();
+ sock = sockfd_lookup(fd, &err);
+ if (!sock)
+ goto out;
+
restart:
- if ((sock = sockfd_lookup(fd, &err))!=NULL)
- {
- if (!(newsock = sock_alloc()))
- {
- err=-EMFILE;
- goto out;
- }
+ err = -EMFILE;
+ if (!(newsock = sock_alloc()))
+ goto out_put;
- inode = newsock->inode;
- newsock->type = sock->type;
+ inode = newsock->inode;
+ newsock->type = sock->type;
- if ((err = sock->ops->dup(newsock, sock)) < 0)
- {
- sock_release(newsock);
- goto out;
- }
-
- err = newsock->ops->accept(sock, newsock, current->files->fd[fd]->f_flags);
+ err = sock->ops->dup(newsock, sock);
+ if (err < 0)
+ goto out_release;
- if (err < 0)
- {
- sock_release(newsock);
- goto out;
- }
- newsock = socki_lookup(inode);
+ err = newsock->ops->accept(sock, newsock, sock->file->f_flags);
+ if (err < 0)
+ goto out_release;
+ newsock = socki_lookup(inode);
- if ((err = get_fd(inode)) < 0)
+ if ((err = get_fd(inode)) < 0)
+ goto out_inval;
+ newsock->file = current->files->fd[err];
+
+ if (upeer_sockaddr)
+ {
+ /* Handle the race where the accept works and we
+ then getname after it has closed again */
+ if(newsock->ops->getname(newsock, (struct sockaddr *)address, &len, 1)<0)
{
- sock_release(newsock);
- err=-EINVAL;
- goto out;
+ sys_close(err);
+ goto restart;
}
+ move_addr_to_user(address, len, upeer_sockaddr, upeer_addrlen);
+ }
- newsock->file = current->files->fd[err];
-
- if (upeer_sockaddr)
- {
- /* Handle the race where the accept works and we
- then getname after it has closed again */
- if(newsock->ops->getname(newsock, (struct sockaddr *)address, &len, 1)<0)
- {
- sys_close(err);
- goto restart;
- }
- move_addr_to_user(address,len, upeer_sockaddr, upeer_addrlen);
- }
+out_put:
+ sockfd_put(sock);
out:
- sockfd_put(sock);
- }
unlock_kernel();
return err;
+
+out_inval:
+ err = -EINVAL;
+out_release:
+ sock_release(newsock);
+ goto out_put;
}
@@ -864,13 +862,17 @@
int err;
lock_kernel();
- if ((sock = sockfd_lookup(fd,&err))!=NULL)
- {
- if((err=move_addr_to_kernel(uservaddr,addrlen,address))>=0)
- err = sock->ops->connect(sock, (struct sockaddr *)address, addrlen,
- current->files->fd[fd]->f_flags);
- sockfd_put(sock);
- }
+ sock = sockfd_lookup(fd, &err);
+ if (!sock)
+ goto out;
+ err = move_addr_to_kernel(uservaddr, addrlen, address);
+ if (err < 0)
+ goto out_put;
+ err = sock->ops->connect(sock, (struct sockaddr *) address, addrlen,
+ sock->file->f_flags);
+out_put:
+ sockfd_put(sock);
+out:
unlock_kernel();
return err;
}
@@ -884,16 +886,20 @@
{
struct socket *sock;
char address[MAX_SOCK_ADDR];
- int len;
- int err;
+ int len, err;
lock_kernel();
- if ((sock = sockfd_lookup(fd, &err))!=NULL)
- {
- if((err=sock->ops->getname(sock, (struct sockaddr *)address, &len, 0))==0)
- err=move_addr_to_user(address,len, usockaddr, usockaddr_len);
- sockfd_put(sock);
- }
+ sock = sockfd_lookup(fd, &err);
+ if (!sock)
+ goto out;
+ err = sock->ops->getname(sock, (struct sockaddr *)address, &len, 0);
+ if (err)
+ goto out_put;
+ err = move_addr_to_user(address, len, usockaddr, usockaddr_len);
+
+out_put:
+ sockfd_put(sock);
+out:
unlock_kernel();
return err;
}
@@ -934,27 +940,29 @@
struct iovec iov;
lock_kernel();
- if ((sock = sockfd_lookup(fd, &err))!=NULL)
- {
- if(len>=0)
- {
- iov.iov_base=buff;
- iov.iov_len=len;
- msg.msg_name=NULL;
- msg.msg_namelen=0;
- msg.msg_iov=&iov;
- msg.msg_iovlen=1;
- msg.msg_control=NULL;
- msg.msg_controllen=0;
- if (current->files->fd[fd]->f_flags & O_NONBLOCK)
- flags |= MSG_DONTWAIT;
- msg.msg_flags=flags;
- err=sock_sendmsg(sock, &msg, len);
- }
- else
- err=-EINVAL;
- sockfd_put(sock);
- }
+ sock = sockfd_lookup(fd, &err);
+ if (!sock)
+ goto out;
+ err = -EINVAL;
+ if (len < 0)
+ goto out_put;
+
+ iov.iov_base=buff;
+ iov.iov_len=len;
+ msg.msg_name=NULL;
+ msg.msg_namelen=0;
+ msg.msg_iov=&iov;
+ msg.msg_iovlen=1;
+ msg.msg_control=NULL;
+ msg.msg_controllen=0;
+ if (sock->file->f_flags & O_NONBLOCK)
+ flags |= MSG_DONTWAIT;
+ msg.msg_flags = flags;
+ err = sock_sendmsg(sock, &msg, len);
+
+out_put:
+ sockfd_put(sock);
+out:
unlock_kernel();
return err;
}
@@ -975,36 +983,37 @@
struct iovec iov;
lock_kernel();
- if ((sock = sockfd_lookup(fd,&err))!=NULL)
+ sock = sockfd_lookup(fd, &err);
+ if (!sock)
+ goto out;
+ iov.iov_base=buff;
+ iov.iov_len=len;
+ msg.msg_name=NULL;
+ msg.msg_iov=&iov;
+ msg.msg_iovlen=1;
+ msg.msg_control=NULL;
+ msg.msg_controllen=0;
+ msg.msg_namelen=addr_len;
+ if(addr)
{
- iov.iov_base=buff;
- iov.iov_len=len;
- msg.msg_name=NULL;
- msg.msg_iov=&iov;
- msg.msg_iovlen=1;
- msg.msg_control=NULL;
- msg.msg_controllen=0;
- msg.msg_namelen=addr_len;
- if(addr)
- {
- err=move_addr_to_kernel(addr,addr_len,address);
- if (err < 0)
- goto bad;
- msg.msg_name=address;
- }
- if (current->files->fd[fd]->f_flags & O_NONBLOCK)
- flags |= MSG_DONTWAIT;
- msg.msg_flags=flags;
- err=sock_sendmsg(sock, &msg, len);
-bad:
- sockfd_put(sock);
+ err = move_addr_to_kernel(addr, addr_len, address);
+ if (err < 0)
+ goto out_put;
+ msg.msg_name=address;
}
+ if (sock->file->f_flags & O_NONBLOCK)
+ flags |= MSG_DONTWAIT;
+ msg.msg_flags = flags;
+ err = sock_sendmsg(sock, &msg, len);
+
+out_put:
+ sockfd_put(sock);
+out:
unlock_kernel();
return err;
}
-
/*
* Receive a frame from the socket and optionally record the address of the
* sender. We verify the buffers are writable and if needed move the
@@ -1021,26 +1030,30 @@
int err,err2;
lock_kernel();
- if ((sock = sockfd_lookup(fd, &err))!=NULL)
- {
- msg.msg_control=NULL;
- msg.msg_controllen=0;
- msg.msg_iovlen=1;
- msg.msg_iov=&iov;
- iov.iov_len=size;
- iov.iov_base=ubuf;
- msg.msg_name=address;
- msg.msg_namelen=MAX_SOCK_ADDR;
- err=sock_recvmsg(sock, &msg, size,
- (current->files->fd[fd]->f_flags & O_NONBLOCK) ? (flags | MSG_DONTWAIT) : flags);
- if(err>=0 && addr!=NULL)
- {
- err2=move_addr_to_user(address, msg.msg_namelen, addr, addr_len);
- if(err2<0)
- err=err2;
- }
- sockfd_put(sock);
- }
+ sock = sockfd_lookup(fd, &err);
+ if (!sock)
+ goto out;
+
+ msg.msg_control=NULL;
+ msg.msg_controllen=0;
+ msg.msg_iovlen=1;
+ msg.msg_iov=&iov;
+ iov.iov_len=size;
+ iov.iov_base=ubuf;
+ msg.msg_name=address;
+ msg.msg_namelen=MAX_SOCK_ADDR;
+ if (sock->file->f_flags & O_NONBLOCK)
+ flags |= MSG_DONTWAIT;
+ err=sock_recvmsg(sock, &msg, size, flags);
+
+ if(err >= 0 && addr != NULL)
+ {
+ err2=move_addr_to_user(address, msg.msg_namelen, addr, addr_len);
+ if(err2<0)
+ err=err2;
+ }
+ sockfd_put(sock);
+out:
unlock_kernel();
return err;
}
@@ -1137,11 +1150,9 @@
lock_kernel();
+ err=-EFAULT;
if (copy_from_user(&msg_sys,msg,sizeof(struct msghdr)))
- {
- err=-EFAULT;
goto out;
- }
/* do not move before msg_sys is valid */
if (msg_sys.msg_iovlen>UIO_MAXIOV)
goto out;
@@ -1166,26 +1177,24 @@
/* Note - when this code becomes multithreaded on
* SMP machines you have a race to fix here.
*/
+ err = -ENOBUFS;
ctl_buf = sock_kmalloc(sock->sk, msg_sys.msg_controllen,
GFP_KERNEL);
if (ctl_buf == NULL)
- {
- err = -ENOBUFS;
goto failed2;
- }
}
+ err = -EFAULT;
if (copy_from_user(ctl_buf, msg_sys.msg_control,
- msg_sys.msg_controllen)) {
- err = -EFAULT;
+ msg_sys.msg_controllen))
goto failed;
- }
msg_sys.msg_control = ctl_buf;
}
msg_sys.msg_flags = flags;
- if (current->files->fd[fd]->f_flags & O_NONBLOCK)
+ if (sock->file->f_flags & O_NONBLOCK)
msg_sys.msg_flags |= MSG_DONTWAIT;
err = sock_sendmsg(sock, &msg_sys, total_len);
+
failed:
if (ctl_buf != ctl)
sock_kfree_s(sock->sk, ctl_buf, msg_sys.msg_controllen);
@@ -1250,7 +1259,7 @@
if ((sock = sockfd_lookup(fd, &err))!=NULL)
{
- if (current->files->fd[fd]->f_flags&O_NONBLOCK)
+ if (sock->file->f_flags & O_NONBLOCK)
flags |= MSG_DONTWAIT;
err=sock_recvmsg(sock, &msg_sys, total_len, flags);
if(err>=0)
@@ -1262,12 +1271,13 @@
if (uaddr != NULL && err>=0)
err = move_addr_to_user(addr, msg_sys.msg_namelen, uaddr, uaddr_len);
- if (err>=0) {
- err = __put_user(msg_sys.msg_flags, &msg->msg_flags);
- if (!err)
- err = __put_user((unsigned long)msg_sys.msg_control-cmsg_ptr,
+ if (err < 0)
+ goto out;
+ err = __put_user(msg_sys.msg_flags, &msg->msg_flags);
+ if (err)
+ goto out;
+ err = __put_user((unsigned long)msg_sys.msg_control-cmsg_ptr,
&msg->msg_controllen);
- }
out:
unlock_kernel();
if(err<0)
--------------7FDB60396CD3222E7DE77675--
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu