3.0.2: rlm_sql_null duplicating its statements

[Stefan Winter]

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/a2a04cd15bc3886ceb7d197d943639365e168652

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0x8A39DC66.asc
Type: application/pgp-keys
Size: 3243 bytes
Desc: not available
URL: <http://lists.freeradius.org/pipermail/freeradius-users/attachments/20140410/2500d704/attachment-0001.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sql-append.patch
Type: text/x-patch
Size: 1102 bytes
Desc: not available
URL: <http://lists.freeradius.org/pipermail/freeradius-users/attachments/20140410/2500d704/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freeradius.org/pipermail/freeradius-users/attachments/20140410/2500d704/attachment-0001.pgp>