2019-06-03 15:27:42

by Joshua Hudson

Subject: O_CLOFORK use case

I ran headlong into the use case for O_CLOFORK and got into a locking
debate over it.

The actual use case involves squashing a thread race between two
threads. If a file is opened for write in one thread with O_CLOEXEC
while another thread calls fork(), a race condition can happen where
the thread that closes the handle misses the out of disk error because
the child process closed the handle last inside execve().

The decades old idiom for replacing config files isn't safe in
multi-threaded code. Yipe.

int h = open(".configfile~", O_WRONY | O_EXCL | O_CLOEXEC, 0666);
if (h < 0) { perror(".configfile"); return 1; }
ssize_t delta = 0;
while ((delta = write(h, newconfigdata, newconfiglen)) > 0) {
newconfigdata += delta;
newconfiglen -= delta;
}
if (delta < 0) { perror(".configfile"); return 1; }
if (close(h)) { perror(".configfile"); return 1; }
rename(".configfile~", ".configfile");

To fix it, we have to put locks around close() and fork()/vfork(). Ugh.

2019-06-03 15:58:51

by Mihai Donțu

Subject: Re: O_CLOFORK use case

On Mon, 2019-06-03 at 08:24 -0700, Joshua Hudson wrote:
> I ran headlong into the use case for O_CLOFORK and got into a locking
> debate over it.
>
> The actual use case involves squashing a thread race between two
> threads. If a file is opened for write in one thread with O_CLOEXEC
> while another thread calls fork(), a race condition can happen where
> the thread that closes the handle misses the out of disk error because
> the child process closed the handle last inside execve().
>
> The decades old idiom for replacing config files isn't safe in
> multi-threaded code. Yipe.
>
> int h = open(".configfile~", O_WRONY | O_EXCL | O_CLOEXEC, 0666);
> if (h < 0) { perror(".configfile"); return 1; }
> ssize_t delta = 0;
> while ((delta = write(h, newconfigdata, newconfiglen)) > 0) {
> newconfigdata += delta;
> newconfiglen -= delta;
> }
> if (delta < 0) { perror(".configfile"); return 1; }
> if (close(h)) { perror(".configfile"); return 1; }
> rename(".configfile~", ".configfile");
>
> To fix it, we have to put locks around close() and fork()/vfork(). Ugh.

fsync() / fdatasync() before close() should fix it, non?

I was under the impression that any of those two became mandatory when
ext4 came along.

--
Mihai Donțu