3.0.2: rlm_sql_null duplicating its statements

[Arran Cudbard-Bell]

On 10 Apr 2014, at 12:45, Stefan Winter <stefan.winter at restena.lu> wrote:

> 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 :-)

It's quite inefficient, you have to strdup the entire buffer just to add
two characters.

fcntl locks are completely irrelevant in this case, there should now be a 
mutex around that call preventing multiple writes to the file by different 
threads...

fcntl locks are not mirrored back to threads within the same process, so
if one thread locks the file, and another thread attempts to acquire the
lock it will succeed. They're not at all useful for synchronisation within
a process, only for notifying other processes that the file, or certain
areas of the file, are locked.

Please check the correct fix was cherry picked.

Note: If you're using multiple instances of rlm_sql, and having them all
log to the same log file, you'll still experience issues. The only fix for
this is some kind of central locking API within the server core.

Arran Cudbard-Bell <a.cudbardb at freeradius.org>
FreeRADIUS Development Team

FD31 3077 42EC 7FCD 32FE 5EE2 56CF 27F9 30A8 CAA2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 881 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.freeradius.org/pipermail/freeradius-users/attachments/20140410/e6df6eae/attachment-0001.pgp>