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@xxxxxxxxx>
>>
>> ---
>>
>> 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
