#13795 closed bug (fixed)

flock may be used multiple times from the same process

Reported by: pulkomandy Owned by: nobody
Priority: normal Milestone: Unscheduled
Component: System/libroot.so Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description (last modified by pulkomandy)

This is supposed to work (ie. the second lock should fail because the first one is held):

#include <fcntl.h>
#include <sys/file.h>
#include <sys/stat.h>

int main(void)
{
        int fd = open("flock", O_TRUNC | O_CREAT | O_WRONLY, 0666);
        int fd2 = open("flock", O_TRUNC | O_CREAT | O_WRONLY, 0666);

        if (flock(fd, LOCK_EX | LOCK_NB) != 0) {
                puts("Failed to grab first lock");
                return -1;
        }

        if (flock(fd2, LOCK_EX | LOCK_NB) == 0) {
                puts("Managed to grab second lock");
                return -1;
        }

        return 0;
}

Basically, flock is not owned by the team, but by the file descriptor. This also explains the behavior with regard to fork().

We are currently using a session_id (pid_t) as an identifier for locks. Could we switch to using the file descriptor number instead, or would that break something? (this would mean the process should keep the fd opened until the lock is released, and should release using the same fd used for locking).

Attachments (1)

fixlocks.patch (4.9 KB) - added by pulkomandy 17 months ago.
Tentative fix (incomplete)

Download all attachments as: .zip

Change History (11)

comment:1 Changed 17 months ago by pulkomandy

Description: modified (diff)

comment:2 Changed 17 months ago by korli

Maybe I'm being pedantic, you should give sources which indicate the behavior you describe.

comment:3 Changed 17 months ago by korli

Tested on Linux, the test exits with 0.

comment:4 Changed 17 months ago by pulkomandy

Ah right, sorry: I hit this while running webkitpy unit tests and also confirmed the behavior on Linux.

From the Linux man page (https://linux.die.net/man/2/flock):

If a process uses open(2) (or similar) to obtain more than one descriptor for the same file, these descriptors are treated independently by flock(). An attempt to lock the file using one of these file descriptors may be denied by a lock that the calling process has already placed via another descriptor.

Changed 17 months ago by pulkomandy

Attachment: fixlocks.patch added

Tentative fix (incomplete)

comment:5 Changed 17 months ago by pulkomandy

Has a Patch: set

comment:6 Changed 17 months ago by pulkomandy

Attached an attempt to fix this. The locks are now namespaced by either team or session id (this seems uncertain in the existing code already) + fd number.

There remains a problem in handling of closing the file, because the fd is not available in file_close. Maybe we should use a pointer to the file_descriptor struct instead? Is that always in sync with the fd?

Locks obtained by fcntl will use an fd of -1, so they behave like if there was a separate file descriptor just for them (this means a process trying to both flock and fcntl-lock the same file would now fail).

Review welcome as I'm not very familiar with this code.

comment:7 Changed 17 months ago by korli

Seeing how Pavel applied a fix to the same function in #8661, he might be a good reviewer. Adding a unit test (in src/tests/..) would also be nice.

comment:8 Changed 15 months ago by pulkomandy

Has a Patch: unset

comment:9 Changed 15 months ago by pulkomandy

Has a Patch: unset

Patch migrated to Gerrit: https://review.haiku-os.org/49

comment:10 Changed 11 months ago by waddlesplash

Has a Patch: unset
Resolution: fixed
Status: newclosed

Patch merged in hrev51970.

Note: See TracTickets for help on using tickets.