[Home]

Summary:ASTERISK-10354: [patch] Doesn't lock config files when writing
Reporter:Faidon Liambotis (paravoid)Labels:
Date Opened:2007-09-21 00:48:52Date Closed:2011-06-07 14:02:50
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/Configuration
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20070922__bug10781.diff.txt
Description:Asterisk does not use flock() to lock configuration files when writing them on config_text_file_save().

This was originally reported as Debian bug #353227 by John Goerzen. In his own words:
"It appears to not protect the file even against itself.  It certainly
doesn't use flock() to protect it against others.

This is a problem for several reasons.  First, of course, people can be
hand-editing the file to add or remove users.  Secondly, automated
programs may be appending data to it for the same purpose.

We've noticed corruption in our file and are almost certain that this is
the culprit."

****** ADDITIONAL INFORMATION ******

I've thought for a moment about adding the locks myself (the code should be trivial enough) and attaching a patch here but that would mean one of the following things:
- Asterisk (i.e. the relevant thread) blocks when the lock is already held, which would result in some bad consequences (voicemail locking anyone?)
- Or Asterisk would use the non-blocking flag and fail if the lock is already being held. That seems sensible but it may surprise the user.
Comments:By: Tilghman Lesher (tilghman) 2007-09-22 12:28:20

This patch pretty well ducks the thorny problem of locking (especially when config files may be on NFS) by creating the file as a temporary file and renaming it at the last minute.  This should prevent any issues with corruption of the configuration file.
By: Faidon Liambotis (paravoid) 2007-09-23 00:07:05

While this would solve any corruption problems, it is still subject to race conditions where only one of the racers will actually write to the file.

Is it possible that two or more config_text_file_save() are being run at the same time in different threads?
By: Tilghman Lesher (tilghman) 2007-09-23 11:19:34

Yes, it is very possible, but there simply is no good way to solve that.  Using file locks will create EXACTLY the same situation as what this patch provides; that is, one of the threads will succeed in rewriting the file, and the other will not.

However, what it will do is to prevent file corruption, which is a valid complaint about the current system.
By: Tilghman Lesher (tilghman) 2007-09-23 12:21:47

Can you think of a scenario where it would behave differently?  If so, I'll work to address that issue.
By: Faidon Liambotis (paravoid) 2007-09-24 01:24:45

No, not exactly.

With file locking a process (or a thread for that matter) can *inform* the others that it's writing the files so they can hold their operation until writing is done.

Your solution is safe -file corruption wise-, even safer than locking.

But it won't block a thread from trying to write a file while it's already being written by another process. Or block another process while Asterisk is writing the file.

OTOH, I'm not sure how well will Asterisk behave if that thread gets blocked for an unknown period of time.

I guess that's only a problem of interaction between Asterisk and other processes (e.g. editors).
By: Tilghman Lesher (tilghman) 2007-09-24 07:22:49

I don't think it's a good idea to block in an interactive thread, as manager connections tend to be.  If we use locking at all, the lock will have a timeout, same as we've implemented locking for voicemail.
By: Tzafrir Cohen (tzafrir) 2007-09-25 01:14:13

Just a reminder: Asterisk is not the only one writing to Asterisk configuration files.

voicemail.conf, specifically, is written to by quite a few config tools.
By: Tilghman Lesher (tilghman) 2007-09-25 17:56:49

tzafrir:  not quite sure how that's relevant.  This technique will prevent corruption on Asterisk's behalf.  There isn't anything we can do to avoid corruption by another utility.
By: Faidon Liambotis (paravoid) 2007-09-26 05:23:06

Well, there is.
Asterisk can call flock(LOCK_EX | LOCK_NB) to try to get an exclusive lock without aborting if this wasn't found.

Sounds a bit evil but this will help with other applications that could use flock and are not interactive (e.g. web interfaces).

So, if Asterisk won't honor exclusive locks the best it can do is hope that others will.
By: Tilghman Lesher (tilghman) 2007-09-26 08:17:42

...which doesn't work if the files are on NFS or if another utility fails to use flock...whereas the current patch will avoid corruption, no matter what another utility does or where the files are located.
By: Faidon Liambotis (paravoid) 2007-09-26 08:19:35

I am aware of flock() issues with NFS and with flock() being voluntarily.

I wasn't suggesting to replace your patch with flock() but merely *add* flock to your patch for those cases where other applications decide to use it and the underlying filesystem supports it.
By: Tilghman Lesher (tilghman) 2007-10-01 10:30:54

Use of flock does not materially affect this implementation in any way whatsoever, other than making the process slower by a factor of two system calls.
By: Tilghman Lesher (tilghman) 2007-10-02 12:55:35

Any testing results whatsoever?
By: Tilghman Lesher (tilghman) 2007-11-12 14:06:23.000-0600

Reporter lost interest.
By: Digium Subversion (svnbot) 2007-11-12 14:14:16.000-0600

Repository: asterisk
Revision: 89191

U   branches/1.4/main/config.c

------------------------------------------------------------------------
r89191 | tilghman | 2007-11-12 14:14:16 -0600 (Mon, 12 Nov 2007) | 5 lines

If two config writes collide, file corruption could result.  Use a mkstemp() file, instead.
Reported by: paravoid
Patch by: tilghman
Closes issue ASTERISK-10354

------------------------------------------------------------------------
By: Digium Subversion (svnbot) 2007-11-12 14:27:33.000-0600

Repository: asterisk
Revision: 89193

_U  trunk/

------------------------------------------------------------------------
r89193 | tilghman | 2007-11-12 14:27:32 -0600 (Mon, 12 Nov 2007) | 12 lines

Blocked revisions 89191 via svnmerge

........
r89191 | tilghman | 2007-11-12 14:16:18 -0600 (Mon, 12 Nov 2007) | 5 lines

If two config writes collide, file corruption could result.  Use a mkstemp() file, instead.
Reported by: paravoid
Patch by: tilghman
Closes issue ASTERISK-10354

........

------------------------------------------------------------------------