Linux Archive

Linux Archive (http://www.linux-archive.org/)
-   ArchLinux Pacman Development (http://www.linux-archive.org/archlinux-pacman-development/)
-   -   pacman-optimize: fix database locking race conditions (http://www.linux-archive.org/archlinux-pacman-development/388975-pacman-optimize-fix-database-locking-race-conditions.html)

Andres P 06-21-2010 05:22 PM

pacman-optimize: fix database locking race conditions
 
On Mon, Jun 21, 2010 at 12:37 PM, Denis A. Altoé Falqueto
<denisfalqueto@gmail.com> wrote:
> On Mon, Jun 21, 2010 at 1:55 PM, Andres P <aepd87@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.

I realize that this doesn't mean squat until I present my sources, but I will
eventually. But the important factor is portability, as I'll explain below.

> 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.

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.

Andres P

Denis A. Altoé Falqueto 06-21-2010 05:44 PM

pacman-optimize: fix database locking race conditions
 
On Mon, Jun 21, 2010 at 2:22 PM, Andres P <aepd87@gmail.com> wrote:
> On Mon, Jun 21, 2010 at 12:37 PM, Denis A. Altoé Falqueto
> <denisfalqueto@gmail.com> wrote:
>> On Mon, Jun 21, 2010 at 1:55 PM, Andres P <aepd87@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
-------------------------------------------


All times are GMT. The time now is 11:13 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.