Hi,
Oh, sorry... It's fixed!
I.e. could you point me to a commit to cherry-pick?
https://github.com/FreeRADIUS/freeradius-server/commit/a2a04cd15bc3886ceb7d1...
I cherry-picked this one and recompiled. Didn't truly solve the issue. I have apporx 5 writes into the file per second, and got garbled statements 2-3 times a day after the fix. My colleague Bruno investigated this further. It looks like the fcntl() locking doesn't catch all corner cases. He writes: c.f.: man 2 fcntl As well as being removed by an explicit F_UNLCK, record locks are automatically released when the process terminates or if it closes any file descriptor referring to a file on which locks are held. This is bad: it means that a process can lose the locks on a file like /etc/passwd or /etc/mtab when for some reason a library function decides to open, read and close it. This means that rad_lockfd() might possibly have no effect: -thread1- -thread2- -thread3- open(logfile) . . rad_lockfd() . . write(query) . . write(";\n") . . . open(logfile) open(logfile) . rad_lockfd() . . rad_lockfd( . write(query) . close() . ) . . write(query) . write(";\n") write(";\n") . close() close() We think we fixed it locally (Bruno's patch attached) by combining both strings into one write() as I suggested earlier. We're aware that this is also no /guarantee/ that the string is written atomically, but the cases where it's not are limited to corner conditions (out of memory, thrashing, ...) and so shouldn't occur during normal operations. Take the patch or leave it, it does seem to work for us :-) Stefan
If not... here's a really simple and stupid suggestion (from someone who didn't write C/C++ since a decade!):
if ((rad_lockfd(fd, len + 2) < 0) || (write(fd, query, len) < 0) || (write(fd, ";\n", 2) < 0)) {
Since the main problem is that the ; and queries somehow get mixed - would not a *single* write that combines the two write()s fix that? I'm thinking of:
... || (write(fd, query + ";\n", len + 2) < 0) ...
Maybe I'm too naive ...
Well, it'd involve allocing an intermediary buffer as you can't do string concatenation like that in C.
There is that other multi variable version of write, but i'm not sure if it's guaranteed to be atomic.
Maybe Alan can comment, he seems to have an encyclopaedic knowledge of C pitfalls :)
-Arran
Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
FD31 3077 42EC 7FCD 32FE 5EE2 56CF 27F9 30A8 CAA2
- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/users.html
-- Stefan WINTER Ingenieur de Recherche Fondation RESTENA - Réseau Téléinformatique de l'Education Nationale et de la Recherche 6, rue Richard Coudenhove-Kalergi L-1359 Luxembourg Tel: +352 424409 1 Fax: +352 422473 PGP key updated to 4096 Bit RSA - I will encrypt all mails if the recipient's key is known to me http://pgp.mit.edu:11371/pks/lookup?op=get&search=0xC0DE6A358A39DC66