Re: [PATCH] flock: add support for using fcntl() with open file description locks

[Rasmus Villemoes <rasmus.villemoes@prevas.dk>]

On 17/04/2024 19.33, Masatake YAMATO wrote:
>> Currently, there is no way for shell scripts to safely access
>> resources protected by POSIX locking (fcntl with the F_SETLK/F_SETLKW
>> commands). For example, the glibc function lckpwdf(), used to
>> protect access to the /etc/shadow database, works by taking a
>> F_SETLKW on /etc/.pwd.lock .
>>
>> Due to the odd semantics of POSIX locking (e.g. released when any file
>> descriptor associated to the inode is closed), we cannot usefully
>> directly expose the POSIX F_SETLK/F_SETLKW commands. However, linux
>> 3.15 introduced F_OFD_SETLK[W], with semantics wrt. ownership and
>> release better matching those of flock(2), and crucially they do
>> conflict with locks obtained via F_SETLK[W]. With this, a shell script
>> can do
>>
>>   exec 4> /etc/.pwd.lock
>>   flock --fcntl-ofd 4
>>   <access/modify /etc/shadow ...>
>>   flock --fcntl-ofd --unlock 4 # or just exit
>>
>> without conflicting with passwd(1) or other utilities that
>> access/modify /etc/shadow.
>>
>> The option name is a bit verbose, and no single-letter shorthand is
>> defined, because this is somewhat low-level and the user really needs
>> to know what he is doing.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>
>> ---
>>
>> Both my autotools and meson fu are weak to non-existing, so I don't
>> know if I've written the "test if the header exposes this macro"
>> correctly.
>>
>> I'm not at all married to the option name. I also considered just
>> making it --fcntl, with the possiblity of making that grow an optional
>> argument (for example --fcntl=posix with plain --fcntl being an alias
>> for --fcntl=ofd) should anyone ever figure out a use for the plain
>> F_SETLK, perhaps just for testing.
>>
>>
>>  configure.ac      |  6 +++++
>>  meson.build       |  3 +++
>>  sys-utils/flock.c | 60 +++++++++++++++++++++++++++++++++++++++++++++--
>>  3 files changed, 67 insertions(+), 2 deletions(-)
> 
> You may have to update sys-utils/flock.1.adoc and the completion rule bash-completion/flock
> when adding a new optoin.

I will send a separate patch with that once the option naming is settled
and the concept itself is accepted (not necessarily applied to master).

>>  code = '''
>>  #include <time.h>
>>  #if !@0@
>> diff --git a/sys-utils/flock.c b/sys-utils/flock.c
>> index 7d878ff81..40751517d 100644
>> --- a/sys-utils/flock.c
>> +++ b/sys-utils/flock.c
>> @@ -70,6 +70,9 @@ static void __attribute__((__noreturn__)) usage(void)
>>  	fputs(_(  " -o, --close              close file descriptor before running command\n"), stdout);
>>  	fputs(_(  " -c, --command <command>  run a single command string through the shell\n"), stdout);
>>  	fputs(_(  " -F, --no-fork            execute command without forking\n"), stdout);
>> +#ifdef HAVE_FCNTL_OFD_LOCKS
>> +	fputs(_(  "     --fcntl-ofd          use fcntl(F_OFD_SETLK) rather than flock()\n"), stdout);
>> +#endif
>>  	fputs(_(  "     --verbose            increase verbosity\n"), stdout);
>>  	fputs(USAGE_SEPARATOR, stdout);
>>  	fprintf(stdout, USAGE_HELP_OPTIONS(26));
>> @@ -126,6 +129,38 @@ static void __attribute__((__noreturn__)) run_program(char **cmd_argv)
>>  	_exit((errno == ENOMEM) ? EX_OSERR : EX_UNAVAILABLE);
>>  }
>>  
>> +static int flock_to_fcntl_type(int op)
>> +{
>> +        switch (op) {
>> +                case LOCK_EX:
>> +                        return F_WRLCK;
>> +                case LOCK_SH:
>> +                        return F_RDLCK;
>> +                case LOCK_UN:
>> +                        return F_UNLCK;
>> +                default:
>> +			errx(EX_SOFTWARE, _("internal error, unknown operation %d"), op);
>> +        }
>> +}
> 
> Don't you need wrap flock_to_fcntl_type with #ifdef HAVE_FCNTL_OFD_LOCKS/#endif?

Well, the constants mentioned here are the same as used with posix
locking, and have been in glibc and kernel headers since forever. Only
the F_OFD_* ones are "newish". So I don't think this needs guarding due
to non-existence of the symbols. But I might need to guard the whole
function (or mark it maybe-unused) to avoid a defined-but-not-used warning.

I wasn't even sure if I actually needed a configure check and HAVE_*
guard at all; 3.15 is 10 years old by now, but I couldn't find any
mention of the oldest glibc/kernel that util-linux is supposed to build
against. Karel?

Rasmus