kern/57374: fcntl(F_GETLK) with SEEK_CUR returns wrong answer when unlocked

[campbell+netbsd%mumble.net@localhost]

>Number:         57374
>Category:       kern
>Synopsis:       fcntl(F_GETLK) with SEEK_CUR returns wrong answer when unlocked
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Apr 22 13:40:00 +0000 2023
>Originator:     Taylor R Campbell
>Release:        current
>Organization:
The NetBSD Flockdation
>Environment:
flocked into a fever
>Description:
When fcntl(F_GETLK) is queried:

- with .l_start = S for some S,
- with .l_whence = SEEK_CUR (meaning: interpret .l_start relative to the file's current offset),
- on a file with no locks,
- on a file with nonzero current offset P,

what it returns is .l_start = S + P, .l_type = F_UNLCK, and .l_whence = SEEK_CUR.

This seems wrong: the returned range may not even overlap the queried range.  It seems to me it should either return:

(a) .l_start = S, .l_type = F_UNLCK, .l_whence = SEEK_CUR; or
(b) .l_start = S + P, .l_type = F_UNLCK, .l_whence = SEEK_SET.

If I'm reading POSIX right, only option (a) is permitted:

> If no lock is found that would prevent this lock from being created, then the structure shall be left unchanged except for the lock type which shall be set to F_UNLCK.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html

The current behaviour happens because the former do_fcntl_lock (now moved to vn_advlock, but nxr has not yet seen this change as of press time) adds the file offset to .l_start in the case .l_whence = SEEK_CUR, but doesn't change .l_whence to reflect this:

    246 	if (fl->l_whence == SEEK_CUR) {
    247 		vn_lock(vp, LK_SHARED | LK_RETRY);
    248 		fl->l_start += fp->f_offset;
    249 		VOP_UNLOCK(vp);
    250 	}

https://nxr.netbsd.org/xref/src/sys/kern/sys_descrip.c?r=1.40#246

And when there is no lock, lf_getlock leaves the other members of the struct flock alone:

    787 	if ((block = lf_getblock(lock)) != NULL) {
...
    799 	} else {
    800 		fl->l_type = F_UNLCK;
    801 	}

https://nxr.netbsd.org/xref/src/sys/kern/vfs_lockf.c?r=1.78#799

And the caller of all this, sys_fcntl, just copies out the struct flock as it exists when do_fcntl_lock returns:

    384 		error = do_fcntl_lock(fd, cmd, &fl);
    385 		if (cmd == F_GETLK && error == 0)
    386 			error = copyout(&fl, SCARG(uap, arg), sizeof(fl));

https://nxr.netbsd.org/xref/src/sys/kern/sys_descrip.c?r=1.40#384
>How-To-Repeat:
$ cat test.c
#include <err.h>
#include <fcntl.h>
#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>

int
main(int argc, char **argv)
{
	struct flock fl = {
		.l_start = 1,
		.l_len = 2,
		.l_pid = getpid(),
		.l_type = F_RDLCK,
		.l_whence = SEEK_CUR,
	};
	int fd;

	setprogname(argv[0]);
	if (argc != 2)
		errx(1, "Usage: %s <file>", getprogname());

	if ((fd = open(argv[1], O_RDWR)) == -1)
		err(1, "open");

	printf("start=%lld len=%lld pid=%lld type=%d whence=%d\n",
	    (long long)fl.l_start,
	    (long long)fl.l_len,
	    (long long)fl.l_pid,
	    fl.l_type,
	    fl.l_whence);
	if (lseek(fd, 3, SEEK_SET) == -1)
		err(1, "lseek");
	if (fcntl(fd, F_GETLK, &fl) == -1)
		err(1, "fcntl(F_GETLK)");
	printf("start=%lld len=%lld pid=%lld type=%d whence=%d\n",
	    (long long)fl.l_start,
	    (long long)fl.l_len,
	    (long long)fl.l_pid,
	    fl.l_type,
	    fl.l_whence);
	if (close(fd) == -1)
		warn("close");
	fflush(stderr);
	return ferror(stderr);
}
$ make test.o test
cc -O2   -c test.c
cc -O2   -o test test.c 
$ ./test test.o
start=1 len=2 pid=20836 type=1 whence=1
start=4 len=2 pid=20836 type=2 whence=1
>Fix:
Yes, please!