Re: [PATCH] please review. file descriptor passing for libc_r.

[Alfred Perlstein]

* Daniel Eischen [EMAIL PROTECTED] [031122 20:46] wrote:
 On Sat, 22 Nov 2003, Alfred Perlstein wrote:
 
  This should make things work properly for apps that are linked
  against libc_r and use filedescriptor passing.
  
  Can someone review and approve it please?
 
 This isn't needed.  Any time a file descriptor is used, a call
 should first be made to _FD_LOCK() which ends up calling
 _thread_fd_lock() which then calls _thread_fd_table_init()
 for the file descriptor.
 
 If there's a reason that this is necessary, then I think it
 is just covering up another problem.  I think I made this
 same comment a couple of years ago ;-)

Sorry for the long delay here, I was in Vegas for my sister's
birthday.

Actually there is a problem here. :(

Descriptor's passed can't be closed because the uthread kernel does this:

int
_close(int fd)
{
int flags;
int ret;
struct stat sb;
struct fd_table_entry   *entry;

if ((fd  0) || (fd = _thread_dtablesize) ||
(fd == _thread_kern_pipe[0]) || (fd == _thread_kern_pipe[1]) ||
(_thread_fd_table[fd] == NULL)) {
/*
 * Don't allow silly programs to close the kernel pipe
 * and non-active descriptors.
 */
errno = EBADF;
ret = -1;
}
...

So basically, if the entry is not initialized we can't close descriptors.

What do you suggest we do?

-- 
- Alfred Perlstein
- Research Engineering Development Inc.
- email: [EMAIL PROTECTED] cell: 408-480-4684
___
[EMAIL PROTECTED] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to [EMAIL PROTECTED]

Re: [PATCH] please review. file descriptor passing for libc_r.

[Alfred Perlstein]

* Alfred Perlstein [EMAIL PROTECTED] [031129 12:21] wrote:
 
 Descriptor's passed can't be closed because the uthread kernel does this:

The weird part is that i'm doing a sendfile(2) using the descriptors
and it appears that sendfile does the FD_LOCK thing on the descriptors
so... ?


 
 int
 _close(int fd)
 {
 int flags;
 int ret;
 struct stat sb;
 struct fd_table_entry   *entry;
 
 if ((fd  0) || (fd = _thread_dtablesize) ||
 (fd == _thread_kern_pipe[0]) || (fd == _thread_kern_pipe[1]) ||
 (_thread_fd_table[fd] == NULL)) {
 /*
  * Don't allow silly programs to close the kernel pipe
  * and non-active descriptors.
  */
 errno = EBADF;
 ret = -1;
 }
 ...
 
 So basically, if the entry is not initialized we can't close descriptors.
 
 What do you suggest we do?
 
 -- 
 - Alfred Perlstein
 - Research Engineering Development Inc.
 - email: [EMAIL PROTECTED] cell: 408-480-4684

-- 
- Alfred Perlstein
- Research Engineering Development Inc.
- email: [EMAIL PROTECTED] cell: 408-480-4684
___
[EMAIL PROTECTED] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to [EMAIL PROTECTED]

Re: [PATCH] please review. file descriptor passing for libc_r.

[Daniel Eischen]

On Sat, 29 Nov 2003, Alfred Perlstein wrote:

 * Daniel Eischen [EMAIL PROTECTED] [031122 20:46] wrote:
  On Sat, 22 Nov 2003, Alfred Perlstein wrote:
  
   This should make things work properly for apps that are linked
   against libc_r and use filedescriptor passing.
   
   Can someone review and approve it please?
  
  This isn't needed.  Any time a file descriptor is used, a call
  should first be made to _FD_LOCK() which ends up calling
  _thread_fd_lock() which then calls _thread_fd_table_init()
  for the file descriptor.
  
  If there's a reason that this is necessary, then I think it
  is just covering up another problem.  I think I made this
  same comment a couple of years ago ;-)
 
 Sorry for the long delay here, I was in Vegas for my sister's
 birthday.
 
 Actually there is a problem here. :(
 
 Descriptor's passed can't be closed because the uthread kernel does this:
 
 int
 _close(int fd)
 {
 int flags;
 int ret;
 struct stat sb;
 struct fd_table_entry   *entry;
 
 if ((fd  0) || (fd = _thread_dtablesize) ||
 (fd == _thread_kern_pipe[0]) || (fd == _thread_kern_pipe[1]) ||
 (_thread_fd_table[fd] == NULL)) {
 /*
  * Don't allow silly programs to close the kernel pipe
  * and non-active descriptors.
  */
 errno = EBADF;
 ret = -1;
 }
 ...
 
 So basically, if the entry is not initialized we can't close descriptors.
 
 What do you suggest we do?

Just close the file:

if ((fd  0) || (fd = _thread_dtablesize) ||
(fd == _thread_kern_pipe[0]) || (fd == _thread_kern_pipe[1])) {
/*
 * Don't allow silly programs to close the kernel pipe
 * and non-active descriptors.
 */
errno = EBADF;
ret = -1;
}
else if (_thread_fd_table[fd] == NULL)
ret = __sys_close(fd);
else {
...
}

-- 
Dan Eischen


___
[EMAIL PROTECTED] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to [EMAIL PROTECTED]

Re: [PATCH] please review. file descriptor passing for libc_r.

[Daniel Eischen]

[re@ removed]

On Sat, 29 Nov 2003, Alfred Perlstein wrote:

 * Alfred Perlstein [EMAIL PROTECTED] [031129 12:21] wrote:
  
  Descriptor's passed can't be closed because the uthread kernel does this:
 
 The weird part is that i'm doing a sendfile(2) using the descriptors
 and it appears that sendfile does the FD_LOCK thing on the descriptors
 so... ?

It should be fine in that case.  Is it not?

-- 
Dan Eischen

___
[EMAIL PROTECTED] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to [EMAIL PROTECTED]

Re: [PATCH] please review. file descriptor passing for libc_r.

[Alfred Perlstein]

* Daniel Eischen [EMAIL PROTECTED] [031129 14:59] wrote:
 [re@ removed]
 
 On Sat, 29 Nov 2003, Alfred Perlstein wrote:
 
  * Alfred Perlstein [EMAIL PROTECTED] [031129 12:21] wrote:
   
   Descriptor's passed can't be closed because the uthread kernel does this:
  
  The weird part is that i'm doing a sendfile(2) using the descriptors
  and it appears that sendfile does the FD_LOCK thing on the descriptors
  so... ?
 
 It should be fine in that case.  Is it not?

Actually what I was seeing was a bug that was compounded by a bug
in my code...

It still appears that the current code will not close a descriptor
that has been passed and has had nothing done to it other than
close().

-- 
- Alfred Perlstein
- Research Engineering Development Inc.
- email: [EMAIL PROTECTED] cell: 408-480-4684
___
[EMAIL PROTECTED] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to [EMAIL PROTECTED]

Re: [PATCH] please review. file descriptor passing for libc_r.

[Alfred Perlstein]

* Daniel Eischen [EMAIL PROTECTED] [031129 14:57] wrote:
  
  What do you suggest we do?
 
 Just close the file:
 
   if ((fd  0) || (fd = _thread_dtablesize) ||
   (fd == _thread_kern_pipe[0]) || (fd == _thread_kern_pipe[1])) {
   /*
* Don't allow silly programs to close the kernel pipe
* and non-active descriptors.
*/
   errno = EBADF;
   ret = -1;
   }
   else if (_thread_fd_table[fd] == NULL)
   ret = __sys_close(fd);
   else {
   ...
   }

So remove the check?  do you approve? does re@ approve?

Index: uthread_close.c
===
RCS file: /home/ncvs/src/lib/libc_r/uthread/uthread_close.c,v
retrieving revision 1.16
diff -u -r1.16 uthread_close.c
--- uthread_close.c 9 Jun 2003 16:45:37 -   1.16
+++ uthread_close.c 29 Nov 2003 23:48:14 -
@@ -50,8 +50,7 @@
struct fd_table_entry   *entry;
 
if ((fd  0) || (fd = _thread_dtablesize) ||
-   (fd == _thread_kern_pipe[0]) || (fd == _thread_kern_pipe[1]) ||
-   (_thread_fd_table[fd] == NULL)) {
+   (fd == _thread_kern_pipe[0]) || (fd == _thread_kern_pipe[1]))
/*
 * Don't allow silly programs to close the kernel pipe
 * and non-active descriptors.

--
- Alfred Perlstein
- Research Engineering Development Inc.
- email: [EMAIL PROTECTED] cell: 408-480-4684
___
[EMAIL PROTECTED] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to [EMAIL PROTECTED]

Re: [PATCH] please review. file descriptor passing for libc_r.

[Daniel Eischen]

On Sat, 29 Nov 2003, Alfred Perlstein wrote:

 * Daniel Eischen [EMAIL PROTECTED] [031129 14:57] wrote:
   
   What do you suggest we do?
  
  Just close the file:
  
  if ((fd  0) || (fd = _thread_dtablesize) ||
  (fd == _thread_kern_pipe[0]) || (fd == _thread_kern_pipe[1])) {
  /*
   * Don't allow silly programs to close the kernel pipe
   * and non-active descriptors.
   */
  errno = EBADF;
  ret = -1;
  }
  else if (_thread_fd_table[fd] == NULL)
  ret = __sys_close(fd);
  else {
  ...
  }
 
 So remove the check?  do you approve? does re@ approve?

You should just call close() if the fd has not been initialized.

Index: uthread_close.c
===
RCS file: /opt/FreeBSD/cvs/src/lib/libc_r/uthread/uthread_close.c,v
retrieving revision 1.16
diff -u -r1.16 uthread_close.c
--- uthread_close.c 9 Jun 2003 16:45:37 -   1.16
+++ uthread_close.c 30 Nov 2003 09:05:52 -
@@ -50,8 +50,7 @@
struct fd_table_entry   *entry;
 
if ((fd  0) || (fd = _thread_dtablesize) ||
-   (fd == _thread_kern_pipe[0]) || (fd == _thread_kern_pipe[1]) ||
-   (_thread_fd_table[fd] == NULL)) {
+   (fd == _thread_kern_pipe[0]) || (fd == _thread_kern_pipe[1])) {
/*
 * Don't allow silly programs to close the kernel pipe
 * and non-active descriptors.
@@ -59,6 +58,8 @@
errno = EBADF;
ret = -1;
}
+   else if (_thread_fd_table[fd] == NULL)
+   ret = __sys_close(fd);
/*
 * Lock the file descriptor while the file is closed and get
 * the file descriptor status:

-- 
Dan Eischen

___
[EMAIL PROTECTED] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to [EMAIL PROTECTED]

Re: [PATCH] please review. file descriptor passing for libc_r.

[Alfred Perlstein]

Seems like a good diff.

* Daniel Eischen [EMAIL PROTECTED] [031129 21:41] wrote:
 
 You should just call close() if the fd has not been initialized.
 
 Index: uthread_close.c
 ===
 RCS file: /opt/FreeBSD/cvs/src/lib/libc_r/uthread/uthread_close.c,v
 retrieving revision 1.16
 diff -u -r1.16 uthread_close.c
 --- uthread_close.c   9 Jun 2003 16:45:37 -   1.16
 +++ uthread_close.c   30 Nov 2003 09:05:52 -
 @@ -50,8 +50,7 @@
   struct fd_table_entry   *entry;
  
   if ((fd  0) || (fd = _thread_dtablesize) ||
 - (fd == _thread_kern_pipe[0]) || (fd == _thread_kern_pipe[1]) ||
 - (_thread_fd_table[fd] == NULL)) {
 + (fd == _thread_kern_pipe[0]) || (fd == _thread_kern_pipe[1])) {
   /*
* Don't allow silly programs to close the kernel pipe
* and non-active descriptors.
 @@ -59,6 +58,8 @@
   errno = EBADF;
   ret = -1;
   }
 + else if (_thread_fd_table[fd] == NULL)
 + ret = __sys_close(fd);
   /*
* Lock the file descriptor while the file is closed and get
* the file descriptor status:
 
 -- 
 Dan Eischen

-- 
- Alfred Perlstein
- Research Engineering Development Inc.
- email: [EMAIL PROTECTED] cell: 408-480-4684
___
[EMAIL PROTECTED] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to [EMAIL PROTECTED]

Re: [PATCH] please review. file descriptor passing for libc_r.

[Daniel Eischen]

On Sat, 22 Nov 2003, Alfred Perlstein wrote:

 This should make things work properly for apps that are linked
 against libc_r and use filedescriptor passing.
 
 Can someone review and approve it please?

This isn't needed.  Any time a file descriptor is used, a call
should first be made to _FD_LOCK() which ends up calling
_thread_fd_lock() which then calls _thread_fd_table_init()
for the file descriptor.

If there's a reason that this is necessary, then I think it
is just covering up another problem.  I think I made this
same comment a couple of years ago ;-)

-- 
Dan Eischen

___
[EMAIL PROTECTED] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to [EMAIL PROTECTED]

Re: [PATCH] please review. file descriptor passing for libc_r.

[Alfred Perlstein]

* Daniel Eischen [EMAIL PROTECTED] [031122 20:46] wrote:
 On Sat, 22 Nov 2003, Alfred Perlstein wrote:
 
  This should make things work properly for apps that are linked
  against libc_r and use filedescriptor passing.
  
  Can someone review and approve it please?
 
 This isn't needed.  Any time a file descriptor is used, a call
 should first be made to _FD_LOCK() which ends up calling
 _thread_fd_lock() which then calls _thread_fd_table_init()
 for the file descriptor.
 
 If there's a reason that this is necessary, then I think it
 is just covering up another problem.  I think I made this
 same comment a couple of years ago ;-)

Oh, ok paranioa on my part then.  thanks!

-- 
- Alfred Perlstein
- Research Engineering Development Inc.
- email: [EMAIL PROTECTED] cell: 408-480-4684
___
[EMAIL PROTECTED] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to [EMAIL PROTECTED]