0
\$\begingroup\$

My program implements a function similar to `dup2'. I would like you to please find any problems with the code and what I could improve.

code:

#include <unistd.h>
#include <sys/types.h>
#include <errno.h>
#include <stdio.h>
#include <string.h>
#include <linux/limits.h>

#define OPEN_MAX 1024

int mydup2(int filedes, int filedes2)
{
   int open_fds[OPEN_MAX];
   int open_count = 0;
   int new_fds;
   int i;
   errno = 0;
   if((open_fds[open_count++] = dup(filedes)) == -1)
      return -1;
   if(filedes2 < 0 || filedes2 > OPEN_MAX)
   {
      errno = EBADF;
      return -1;
   }
   if(filedes == filedes2)
   {
      return filedes;
   }
   if((open_fds[open_count++] = dup(filedes2)) >= 0)
   {
      if(close(filedes2) == -1)
      {                                                                                     return -1;
      }
   }
   new_fds = dup(filedes);
   open_fds[open_count++] = new_fds;
   while(new_fds != -1 && new_fds != filedes2)
   {
      open_fds[open_count++] = new_fds;
      new_fds = dup(filedes);
   }
   for(i = 0; i < open_count - 1; ++i)
   {
      close(open_fds[i]);
   }
   return new_fds;
}

void test_mydup2(int (*fun_dup2)(int,int))
{
   int old_fds[] = {0, 0, 2,  0, -1,  10,    -1, 65536};
   int new_fds[] = {0, 2, 2, 30,  2,  20, 65536, 65536};
   int count =  sizeof(old_fds)/sizeof(int);
   int ret;
   int i;
   for(i = 0; i < count; i++)
   {
      printf("dup2(%d, %d) : ", old_fds[i], new_fds[i]);
      ret = fun_dup2(old_fds[i], new_fds[i]);
      if (ret == -1)
      {
         printf("%s\n", strerror(errno));
      }
      else if (ret == new_fds [i])
      {
         printf("success.\n");
     if(ret != 0 && ret != 1 && ret != 2)
            close(ret);
      }
      else
      {
         printf("failed, not equal.\n");
      }                                                                             
   }    
   printf("dup(1, 20), write(20, \"A test\\n\", 7) : ");
   fflush(stdout); 
   fun_dup2(1, 20);
   write(20, "A test\n", 7);
   printf("\n");
   close(20);
}

int main()
{
   printf("********* my dup2 ***********\n");
   test_mydup2(mydup2);
   printf("********standard dup2********\n");
   test_mydup2(dup2);
   return 0;
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ This doesn't look like your own code. It is identical to this code. \$\endgroup\$
    – G. Sliepen
    Aug 4, 2022 at 20:43

1 Answer 1

3
\$\begingroup\$
  1. The first dup is essentially ignored except to check for errors.

    If it actually returned filedes2, your function will fail by accidentally succeeding first try and never checking it.

  2. The first dup is called before the first two sanity checks (filedes2 < 0 || filedes2 > OPEN_MAX and filedes == filedes2).

    In addition to the bug above, if either sanity check fails, the dup'd fd will also be leaked, because you return without executing your cleanup code.

  3. This code duplicates the first entry in open_fds (after the initial one we already addressed):

    new_fds = dup(filedes);
    open_fds[open_count++] = new_fds;
    while(new_fds != -1 && new_fds != filedes2)
    {
      open_fds[open_count++] = new_fds;
      new_fds = dup(filedes);
    }
    

    You should be able to see this by reading the code, or otherwise by stepping through in the debugger or printing out the open_fds array at the end. Swapping the two statements in the body of the while loop will fix that.


Finally,

My program implements a function similar to `dup2'.

isn't really true. You're emulating dup2, but the implementation is entirely different. The library code for dup2 (or the kernel syscall it invokes) has direct access to the file descriptor table and is very unlikely to call dup 1,000 times until it gets lucky.

\$\endgroup\$
2
  • \$\begingroup\$ Being pedantic: It's not the library code for dup2() that has access to the file descriptor table, but the kernel's implementation of the corresponding system call that has it. \$\endgroup\$
    – G. Sliepen
    Aug 4, 2022 at 19:57
  • \$\begingroup\$ Well, I don't know exactly where the libc/syscall boundary will sit on all platforms. If your libc is effectively emulating the POSIX-like interface on a completely different kernel, it could manage its own file descriptors as an indirection layer. But in general of course you're correct. \$\endgroup\$
    – Useless
    Aug 5, 2022 at 12:02

Not the answer you're looking for? Browse other questions tagged or ask your own question.