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
diff -u -r1.1.1.1 smbrun.c
--- lib/smbrun.c	2000/10/28 11:47:21
+++ 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"));
       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

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

But still, function cli_reestablish_connection() is somewhat

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
diff -u -r1.1.1.2 cliconnect.c
--- libsmb/cliconnect.c	2000/10/28 14:20:45
+++ 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
 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

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
diff -u -r1.1.1.1 shared.c
--- smbwrapper/shared.c	2000/10/28 11:47:26
+++ 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