Another bug, in dup3

Another bug, in dup3

[Richard W.M. Jones]

Al:

A second problem we noticed from the gnulib test is that this
assertion fails with the latest upstream kernel:

      /* Assigning to self is invalid.  */
      errno = 0;
      ASSERT (dup3 (fd, fd, o_flags) == -1);   <--- fails
      ASSERT (errno == EINVAL);

dup3 is not currently specified by POSIX, so technically it is fine
for dup3 on Linux to do anything it likes.

*However* the proposal for dup3 in POSIX:

  http://austingroupbugs.net/view.php?id=411

says:

  "The dup3( ) function shall be equivalent to the dup2( ) function if
  the flag argument is 0, except that it shall be an error if fildes
  is equal to fildes2."

So it's probably a good idea for Linux not to regress here.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v

Re: Another bug, in dup3

[Richard W.M. Jones]

[-- Attachment #1: Type: text/plain, Size: 304 bytes --]

Simple reproducer attached.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v

[-- Attachment #2: dup3.c --]
[-- Type: text/plain, Size: 423 bytes --]

/* Test if dup3 works right.
 * by Richard W.M. Jones.
 */

#define _GNU_SOURCE

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>
#include <assert.h>

int
main ()
{
  char *file = "/tmp/dup3.tmp";
  int fd = creat (file, 0600);

  assert (fd >= 0);

  if (dup3 (fd, fd, 0) != -1) {
    fprintf (stderr, "dup3 (fd, fd, 0) didn't fail!\n");
    exit (EXIT_FAILURE);
  }

  exit (EXIT_SUCCESS);
}

[PATCH regression] dup3: Return an error when oldfd == newfd.

[Richard W.M. Jones]

I have tested the attached patch to fix the dup3 regression.

Rich.

>From 0944e30e12dec6544b3602626b60ff412375c78f Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Tue, 9 Oct 2012 14:42:45 +0100
Subject: [PATCH] dup3: Return an error when oldfd == newfd.

The following commit:

  commit fe17f22d7fd0e344ef6447238f799bb49f670c6f
  Author: Al Viro <viro@zeniv.linux.org.uk>
  Date:   Tue Aug 21 11:48:11 2012 -0400

    take purely descriptor-related stuff from fcntl.c to file.c

was supposed to be just code motion, but it dropped the following two
lines:

  if (unlikely(oldfd == newfd))
          return -EINVAL;

from the dup3 system call.  dup3 is not specified by POSIX, so Linux
can do what it likes.  However the POSIX proposal for dup3 [1] states
that it should return an error if oldfd == newfd.

[1] http://austingroupbugs.net/view.php?id=411

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
Tested-by: Richard W.M. Jones <rjones@redhat.com>
---
 fs/file.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/file.c b/fs/file.c
index 0f1bda4..d3b5fa8 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -922,6 +922,9 @@ SYSCALL_DEFINE3(dup3, unsigned int, oldfd, unsigned int, newfd, int, flags)
 	if ((flags & ~O_CLOEXEC) != 0)
 		return -EINVAL;
 
+	if (unlikely(oldfd == newfd))
+		return -EINVAL;
+
 	if (newfd >= rlimit(RLIMIT_NOFILE))
 		return -EMFILE;
 
-- 
1.7.10.4

Re: [PATCH regression] dup3: Return an error when oldfd == newfd.

[Al Viro]

On Tue, Oct 09, 2012 at 03:27:43PM +0100, Richard W.M. Jones wrote:
> I have tested the attached patch to fix the dup3 regression.
> 
> Rich.
> 
> From 0944e30e12dec6544b3602626b60ff412375c78f Mon Sep 17 00:00:00 2001
> From: "Richard W.M. Jones" <rjones@redhat.com>
> Date: Tue, 9 Oct 2012 14:42:45 +0100
> Subject: [PATCH] dup3: Return an error when oldfd == newfd.
> 
> The following commit:
> 
>   commit fe17f22d7fd0e344ef6447238f799bb49f670c6f
>   Author: Al Viro <viro@zeniv.linux.org.uk>
>   Date:   Tue Aug 21 11:48:11 2012 -0400
> 
>     take purely descriptor-related stuff from fcntl.c to file.c
> 
> was supposed to be just code motion, but it dropped the following two
> lines:
> 
>   if (unlikely(oldfd == newfd))
>           return -EINVAL;

*grumble*
So it has; applied, will push to Linus.  But now I really want to check
if I'd lost anything else in those code moves (and you are right, it
was supposed to be just that).