[pacman-dev] [PATCH 2/2] pacman-optimize: fix database locking race conditions

[Denis A. Altoé Falqueto]

On Mon, Jun 21, 2010 at 2:22 PM, Andres P <aepd87 at gmail.com> wrote:
> On Mon, Jun 21, 2010 at 12:37 PM, Denis A. Altoé Falqueto
> <denisfalqueto at gmail.com> wrote:
>> On Mon, Jun 21, 2010 at 1:55 PM, Andres P <aepd87 at gmail.com> wrote:
>>> Instead of making the lock file with touch, use mkdir since it's the only
>>> portable atomic transaction available to shell scripts.
>>
>> In fact, there is another way to do an atomic operation. Look at
>> repo-add script, in function check_repo_db (I've seen this advice in
>> some article some time ago too..). The first command tries to create a
>> file with set -o noclobber. So, if the file already exists, it will
>> fail and will not alter the lock file. If it doesn't exist, the lock
>> file will be created and the process will proceed.
>>
>> if (set -o noclobber; echo "$$" > "$LOCKFILE") 2>/dev/null; then
>>  # We acquired the lock
>> else
>>  # The lock already exists and we can't proceed.
>> fi
>>
> Redirections are not atomic.
>
> First of all, echo isn't atomic so the ret val is old.
>
> Second, even when using this
> $ true > lock
> or simply
> $ > lock
> is still not atomic.

Sure, there're not atomic operations in bash, but the important thing
there is the setting -o noclobber. The code path that can lead to race
condition is only the redirection, instead of the check-and-act normal
way of doing locking. In fact, if it were not for the filesystem
implementing the real creation of the directory, mkdir would not be
atomic too.

>> This would avoid changing the db.lock to a directory. Maybe this could
>> break some kind of script or utility out there...
>>
>
> That would be a bug with that script in particular, because pacman does not
> care if it's a directory or not, as it should.
>
> The lock file in acpid, to name one of the many programs that smartly implement
> directories as lockfiles, is done this way for a reason.
>
> No clobber isn't available in all shells, so you are assuming that only
> bash/ksh will be interacting with the lockfile.

Yes, I like portability too, but pacman-optimize starts with a
#!/bin/bash, so we are actually using bash anyway, even if the user
has a different default shell.

>
> makepkg and similar bash scripts in the source distribution aren't the only
> ways to interact with pacman. As proof, install scripts are supposed to be sh
> compatible.
>
> So we should keep compat with sh when it comes to something as low level and
> crucial as locking the db.

I don't want to argue or anything, just wanted to shed some light on a
different implementation that would not create a new behavior
(directory instead of file). I don't have any counter examples for
your code, as it really works as expected :)

-- 
A: Because it obfuscates the reading.
Q: Why is top posting so bad?

-------------------------------------------
Denis A. Altoe Falqueto
-------------------------------------------