Implementing dup2 in xv6

Question

I'm taking an OS class and we have to implement dup2() in xv6. The code I've written should theoretically work, but, when I try to execute tests, it doesn't pass all of them.

This is my system call:

int sys_dup2(void) {
  int oldfd, newfd;
  struct file *oldf, *newf;  

  struct proc *curproc = myproc();

  if (argfd(0, &oldfd, &oldf) < 0)
    return -1;
  if (argfd(1, &newfd, &newf) < 0)
    return -1;

  if (oldfd == newfd)
    return newfd;
  if (newfd != 0)
    fileclose(newf);

  filedup(oldf);
  curproc->ofile[newfd] = oldf;
  return newfd;
}

The tests that doesn't pass are:

if (dup2 (1,4) != 4)
  printf (2, "dup2 not working with existing fd.\n");

printf (4, "This message outputs on terminal.\n");

if (dup2 (4,6) != 6)
  printf (2, "dup2 not working with existing fd (2).\n");

I'm thinking that the problem is linked with argfd() because when I try to debug with GDB I find oldfd equals to some random number.

This is xv6's argfd():

static int
argfd(int n, int *pfd, struct file **pf)
{
  int fd;
  struct file *f;

  if(argint(n, &fd) < 0)
    return -1;
  if(fd < 0 || fd >= NOFILE || (f=myproc()->ofile[fd]) == 0)
    return -1;
  if(pfd)
    *pfd = fd;
  if(pf)
    *pf = f;
  return 0;
}

I leave the xv6's functions argint(), fetchint() and filedup() to better understand the code above.

int
argint(int n, int *ip)
{
  return fetchint((myproc()->tf->esp) + 4 + 4*n, ip);
}

int
fetchint(uint addr, int *ip)
{
  struct proc *curproc = myproc();

  if(addr >= curproc->sz || addr+4 > curproc->sz)
    return -1;
  *ip = *(int*)(addr);
  return 0;
}

struct file*
filedup(struct file *f)
{
  acquire(&ftable.lock);
  if(f->ref < 1)
    panic("filedup");
  f->ref++;
  release(&ftable.lock);
  return f;
}

Thank you for helping!

Answer 1

dup2(oldfd, newfd) is supposed to work whether newfd is open or not. It looks to me like argfd will fail if called with a file descriptor that is not open (presumably the corresponding entry in the myproc()->ofile table would be null), in which case your function just returns -1, which is wrong. So you won't be able to use argfd to validate the newfd argument. Maybe there's some other standard function available, or maybe you just have to do the same checks for yourself.

It's not clear in the test if fd 4 is supposed to already be open or not (does "existing fd" mean the old one or the new one?) but if it's not, then it does seem like your implementation would fail the test.

The test if (newfd != 0) also looks wrong; 0 is a valid file descriptor (standard input), so it shouldn't be treated differently. In fact you need to dup2(fd, 0) every time you use < input redirection in your shell. I'm not sure what you meant to put here, but it probably has to change anyway when you redo the validation.

Finally, I don't know what xv6's rules are for concurrency and threading, but I'm worried about races and TOCTOU bugs. You don't hold any locks between checking the validity/status of the file descriptors and doing your actual work.

What if another thread of this process closes oldfd or newd after you test it, and deallocates the struct file? Then your struct file * is dangling.

What if another thread is accessing newfd while all this is going on? The dup2 operation is supposed to be atomic, so write(newfd, data, sz) in another thread should write either to the original newfd or the dup'd oldfd, but it should not fail with EBADF. Which is what will happen in your code if that write should occur in the window between fileclose(newf) and curproc->ofile[newfd] = oldf;.