bug related to dup2().
okuyamak at dd.iij4u.or.jp
okuyamak at dd.iij4u.or.jp
Sat Oct 28 17:35:50 GMT 2000
Dear all,
In SAMBA_2_2, there's three "very clear" bug related to system call
dup2. This bug existed from Samba 2.0.7, in same function( I don't
mean same FILE, for some function have moved to new file in 2.2 ),
which is not reported so far, and so, means this is not major path
for Samba. But since this is so CLEARLY bug, I'd like to point it
out here, with fixing patches.
--------------------
./source/lib/smbrun.c: setup_stdout_file()
'dup2' will return "new descriptor" if succeed. And if failed, it will
return -1. Since so, with current code, this function will always
return False.
In this case, new descriptor is "1" not "0", and since so, dup2
should check whether return is "1" or not.
Index: lib/smbrun.c
===================================================================
RCS file: /cvs/Samba-2.2/source/lib/smbrun.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 smbrun.c
--- lib/smbrun.c 2000/10/28 11:47:21 1.1.1.1
+++ lib/smbrun.c 2000/10/28 14:48:38
@@ -58,7 +58,7 @@
if (fd == -1) return False;
if (fd != 1) {
- if (dup2(fd,1) != 0) {
+ if (dup2(fd,1) != 1) {
DEBUG(2,("Failed to create stdout file descriptor\n"));
close(fd);
return False;
--------------------
./source/libsmb/cliconnect.c: cli_reestablish_connection()
Old code is un-understandable.
dup2's semantics are as follows:
int dup2( origfd, newfd )
If dup2 had 'newfd' with opened file descriptor, it will first
close this file.
If dup2 succeed, it will return 'newfd', or '-1' if failed.
Old code duplicates cli->fd to oldfd. If returned value is "oldfd",
which is "usually true", it will close 'cli->fd', then simply return
with variable 'True' ( i.e. value stored as "oldfd" will simply be
gone, for "oldfd" is auto variable, and nobody else is keeping it ).
Also, after returned from this function, 'cli->fd' is no longer
valid. for we already close()-ed it, but since we did not re-set
cli->fd to -1, caller function have dangerous chance of using this
INVALID FILE DESCRIPTOR.
The only reason this code have not harmed system, may be because
this function was never being called.
I've changed old code to simply close "oldfd" which was OLD
'cli->fd' value. New 'cli->fd' holds valid
"cli_establish_connection()" -ed variable, for this function returns
True.
But still, function cli_reestablish_connection() is somewhat
rediculous.
It first keep old cli->fd to oldfd, but for what reason?
cli_establish_connection() do not use this information, we don't
have to care whether old 'cli' contains valid file descriptor or
not. We only need to close() old 'cli->fd', then open the new one.
So what it should really do is:
1) close cli->fd.
2) set cli->fd to -1.
3) call cli_establish_connection() to create new cli.
4) return according to succession.
if, this function is really "reestablish"-ing connection.
The reason why I did not code like so, is because I might be
misunderstanding what this function is really doing.
# buggy code is really hard to understand, for it's not only is
# wrong code, but will hide real purpose of the code ( which
# sometime does not even exist at all ).
Index: libsmb/cliconnect.c
===================================================================
RCS file: /cvs/Samba-2.2/source/libsmb/cliconnect.c,v
retrieving revision 1.1.1.2
diff -u -r1.1.1.2 cliconnect.c
--- libsmb/cliconnect.c 2000/10/28 14:20:45 1.1.1.2
+++ libsmb/cliconnect.c 2000/10/28 15:03:10
@@ -627,9 +627,7 @@
&calling, &called,
share, dev, False, do_tcon)) {
if (cli->fd != oldfd) {
- if (dup2(cli->fd, oldfd) == oldfd) {
- close(cli->fd);
- }
+ close( oldfd );
}
return True;
}
--------------------
./source/smbwrapper/shared.c: smbw_setup_shared()
Original smbw_setup_shared() was something totally nonsense.
Here's what it was doing:
1) create working file.
2) unlink working file.
3) set max files.
4) do dup2(), see if you can duplicate to maximum fd.
Here's why this code is nonsense:
I ) This code should first set max files. As is shown in 4,
this code seems like it wants to limit opening files to
SMBW_MAX_OPEN.
II) open() will return you 'smallest file descriptor' availabe,
if it suceed in opening file. So, if returned file descriptor
was smaller then SMBW_MAX_OPEN, then system is already giving
you valid fd. There's no reason you should dup2() to check
if it fits within SMBW_MAX_OPEN.
III) dup2() might harm system. As is given above, dup2() will
"close" file descriptor given as 'newfd', if it was under use.
So, while() statement running dup2() have high possibility of
closeing file descriptor already in use.
IV) dup2() will open newfd. But THIS NEW FILE might be the
'over-limit' one. We should check about maximum opened file
without dup2()-ing.
V ) This function uses mktemp(), which have famous bug:
mktemp() will return filename that are not being used.
But, this 'not being used' fact may not be true by the time
open() is being called, for unix is multi-process system.
So, very fact that you failed in open()-ing the file, does not
mean system have faild.
But at same time, you might be very unlucky, and will continuously
fail in open()-ing the temporary file. So, you should limit
the count you'll try to open this temporary file.
Since we'll simply 'unlink' this file as soon as we open() it,
there's no limit in filename. So, if we have mkstemp() function,
we should use mkstemp() instead.
VI) 'O_TRUNC' flags for sys_open() is useless. And on some system,
might kick system's bug. If you open() file with 'O_CREAT|O_EXCL'
system will FAIL in opening file if it exists.
And this should work, for mktemp() will look for 'unused' file name.
Non existing file should have size 0, which does not require
truncation.
So, here's what I've coded:
1) set max files to SMBW_MAX_OPEN
2) create template for this sharing file name.
3) try to open temporary file with ( O_RDWR | O_CREAT | O_EXCL ).
until we ether succeed, or failed MAX_TRY_OPEN_TIMES times.
4) if we failed in 3, goto failed.
5) unlink() filename we've just opened.
6) since open() returns 'smallest unused file descriptor number'
we can always tell that if ( fd >= max_fd ), then we're opening
too much. in this case, close fd and goto failed.
7) set shared_fd to fd.
8) set environment variable SMBW_HANDLE to fd.
# I did not use mkstemp() for now, for current 'configure' does not
# check for existance of mkstemp().
Index: smbwrapper/shared.c
===================================================================
RCS file: /cvs/Samba-2.2/source/smbwrapper/shared.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 shared.c
--- smbwrapper/shared.c 2000/10/28 11:47:26 1.1.1.1
+++ smbwrapper/shared.c 2000/10/28 15:27:02
@@ -32,37 +32,46 @@
*******************************************************/
void smbw_setup_shared(void)
{
- int fd;
- pstring s, name;
+#define MAX_TRY_OPEN_TIMES 16
- slprintf(s,sizeof(s)-1, "%s/smbw.XXXXXX",tmpdir());
-
- fstrcpy(name,(char *)smbd_mktemp(s));
-
- /* note zero permissions! don't change this */
- fd = sys_open(name,O_RDWR|O_CREAT|O_TRUNC|O_EXCL,0);
- if (fd == -1) goto failed;
- unlink(name);
-
- shared_fd = set_maxfiles(SMBW_MAX_OPEN);
-
- while (shared_fd && dup2(fd, shared_fd) != shared_fd) shared_fd--;
-
- if (shared_fd == 0) goto failed;
-
- close(fd);
-
- DEBUG(4,("created shared_fd=%d\n", shared_fd));
-
- slprintf(s,sizeof(s)-1,"%d", shared_fd);
-
- smbw_setenv("SMBW_HANDLE", s);
-
- return;
-
- failed:
- perror("Failed to setup shared variable area ");
- exit(1);
+ pstring s, name;
+ int try_times;
+ int max_fd;
+ int fd;
+
+ max_fd = set_maxfiles( SMBW_MAX_OPEN );
+
+ slprintf( s, sizeof( s ) - 1, "%s/smbw.XXXXXX", tmpdir() );
+
+ for ( try_times = 0, fd = -1;
+ ( try_times < MAX_TRY_OPEN_TIMES )&&( fd == -1 ); try_times++ ) {
+ strncpy( name, s, sizeof( name ) - 1 );
+ name[sizeof(s)-1] = '\0';
+ smbd_mktemp( name );
+ fd = sys_open( name, O_RDWR | O_CREAT | O_EXCL, 0 );
+ }
+ if ( fd == -1 ) {
+ goto failed;
+ }
+
+ unlink( name );
+ if ( fd >= max_fd ) {
+ /* we are opening too many files */
+ close( fd );
+ goto failed;
+ }
+
+ shared_fd = fd;
+
+ DEBUG(4,("created shared_fd=%d\n", shared_fd));
+
+ slprintf(s,sizeof(s)-1,"%d", shared_fd);
+ smbw_setenv("SMBW_HANDLE", s);
+ return;
+
+ failed:
+ perror("Failed to setup shared variable area ");
+ exit(1);
}
static int locked;
----
Kenichi Okuyama at Tokyo Research Lab. IBM-Japan. Co. Jp.
More information about the samba-technical
mailing list