[PATCH] libsemanage: remove lock files

[PATCH] libsemanage: remove lock files

[Guido Trentalancia]

Remove semanage read and transaction lock files upon releasing
them.

Signed-off-by: Guido Trentalancia <guido@trentalancia.net>
---
 src/semanage_store.c |    2 ++
 1 file changed, 2 insertions(+)

diff -pruN a/src/semanage_store.c b/src/semanage_store.c
--- a/src/semanage_store.c	2016-10-14 17:31:26.000000000 +0200
+++ b/src/semanage_store.c	2017-04-03 09:32:24.093627962 +0200
@@ -1904,6 +1904,7 @@ void semanage_release_trans_lock(semanag
 		close(sh->u.direct.translock_file_fd);
 		sh->u.direct.translock_file_fd = -1;
 	}
+	unlink(semanage_files[SEMANAGE_TRANS_LOCK]);
 	errno = errsv;
 }
 
@@ -1917,6 +1918,7 @@ void semanage_release_active_lock(semana
 		close(sh->u.direct.activelock_file_fd);
 		sh->u.direct.activelock_file_fd = -1;
 	}
+	unlink(semanage_files[SEMANAGE_READ_LOCK]);
 	errno = errsv;
 }

Re: [PATCH] libsemanage: remove lock files

[Stephen Smalley]

On Thu, 2017-04-20 at 16:38 +0200, Guido Trentalancia wrote:
> Remove semanage read and transaction lock files upon releasing
> them.

Why?

> 
> Signed-off-by: Guido Trentalancia <guido@trentalancia.net>
> ---
>  src/semanage_store.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff -pruN a/src/semanage_store.c b/src/semanage_store.c
> --- a/src/semanage_store.c	2016-10-14 17:31:26.000000000 +0200
> +++ b/src/semanage_store.c	2017-04-03 09:32:24.093627962 +0200
> @@ -1904,6 +1904,7 @@ void semanage_release_trans_lock(semanag
>  		close(sh->u.direct.translock_file_fd);
>  		sh->u.direct.translock_file_fd = -1;
>  	}
> +	unlink(semanage_files[SEMANAGE_TRANS_LOCK]);
>  	errno = errsv;
>  }
>  
> @@ -1917,6 +1918,7 @@ void semanage_release_active_lock(semana
>  		close(sh->u.direct.activelock_file_fd);
>  		sh->u.direct.activelock_file_fd = -1;
>  	}
> +	unlink(semanage_files[SEMANAGE_READ_LOCK]);
>  	errno = errsv;
>  }

Re: [PATCH] libsemanage: remove lock files

[Guido Trentalancia]

Hello Stephen.

Usually, when a lock file is released, the corresponding file is removed from the filesystem for keeping it clean and tidy.

I might be wrong... But why not ?

If nothing is handling the semanage store, then there shouldn't be a reason for keeping it locked. The presence of a lock file, usually means that the lock is active.

Regards,

Guido

> On the 20th of April 2017 alle 17.44 Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
> 
> On Thu, 2017-04-20 at 16:38 +0200, Guido Trentalancia wrote:
> > Remove semanage read and transaction lock files upon releasing
> > them.
> 
> Why?
> 
> > 
> > Signed-off-by: Guido Trentalancia <guido@trentalancia.net>
> > ---
> >  src/semanage_store.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff -pruN a/src/semanage_store.c b/src/semanage_store.c
> > --- a/src/semanage_store.c	2016-10-14 17:31:26.000000000 +0200
> > +++ b/src/semanage_store.c	2017-04-03 09:32:24.093627962 +0200
> > @@ -1904,6 +1904,7 @@ void semanage_release_trans_lock(semanag
> >  		close(sh->u.direct.translock_file_fd);
> >  		sh->u.direct.translock_file_fd = -1;
> >  	}
> > +	unlink(semanage_files[SEMANAGE_TRANS_LOCK]);
> >  	errno = errsv;
> >  }
> >  
> > @@ -1917,6 +1918,7 @@ void semanage_release_active_lock(semana
> >  		close(sh->u.direct.activelock_file_fd);
> >  		sh->u.direct.activelock_file_fd = -1;
> >  	}
> > +	unlink(semanage_files[SEMANAGE_READ_LOCK]);
> >  	errno = errsv;
> >  }

Re: [PATCH] libsemanage: remove lock files

[Stephen Smalley]

On Thu, 2017-04-20 at 17:45 +0200, Guido Trentalancia wrote:
> Hello Stephen.
> 
> Usually, when a lock file is released, the corresponding file is
> removed from the filesystem for keeping it clean and tidy.
> 
> I might be wrong... But why not ?
> 
> If nothing is handling the semanage store, then there shouldn't be a
> reason for keeping it locked. The presence of a lock file, usually
> means that the lock is active.

libsemanage doesn't use the lock files that way; it just uses them as
the object for flock() operations.  So the presence of the lock file
means nothing.  Removing it just means it will have to be re-created on
the next operation.  Not fundamentally opposed, but someone would need
to validate that it doesn't cause any issues.  It's been that way
forever.  Maybe the original Tresys authors of this code have an
opinion on it.

Re: [PATCH] libsemanage: remove lock files

[Guido Trentalancia]

Hello and thanks for getting back.

If it doesn't have any side-effect (as it should), then I think it's preferable that the filesystem is kept clean.

It can be confusing too: because lock files are generally considered "active" when present in the filesystem.

Well, you've heard my opinion and you have the very simple patch now. Feel free to do whatever you and the authors like with it...

Regards,

Guido

> On the 20th of April 2017 at 17.56 Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
> 
> On Thu, 2017-04-20 at 17:45 +0200, Guido Trentalancia wrote:
> > Hello Stephen.
> > 
> > Usually, when a lock file is released, the corresponding file is
> > removed from the filesystem for keeping it clean and tidy.
> > 
> > I might be wrong... But why not ?
> > 
> > If nothing is handling the semanage store, then there shouldn't be a
> > reason for keeping it locked. The presence of a lock file, usually
> > means that the lock is active.
> 
> libsemanage doesn't use the lock files that way; it just uses them as
> the object for flock() operations.  So the presence of the lock file
> means nothing.  Removing it just means it will have to be re-created on
> the next operation.  Not fundamentally opposed, but someone would need
> to validate that it doesn't cause any issues.  It's been that way
> forever.  Maybe the original Tresys authors of this code have an
> opinion on it.

Re: [PATCH] libsemanage: remove lock files

[Stephen Smalley]

On Thu, 2017-04-20 at 17:56 +0200, Guido Trentalancia wrote:
> Hello and thanks for getting back.
> 
> If it doesn't have any side-effect (as it should), then I think it's
> preferable that the filesystem is kept clean.
> 
> It can be confusing too: because lock files are generally considered
> "active" when present in the filesystem.
> 
> Well, you've heard my opinion and you have the very simple patch now.
> Feel free to do whatever you and the authors like with it...

Hmm..see for example the "DON'T unlink" section of:
http://world.std.com/~swmcd/steven/tech/flock.html

Re: [PATCH] libsemanage: remove lock files

[Guido Trentalancia]

Yes, I think you are right, it might lead to a race condition because it uses flock() already.

It is better to leave things as they are.

Please skip this patch !

Regards,

Guido

> On the 20th of April 2017 at 17.56 Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
> 
> On Thu, 2017-04-20 at 17:45 +0200, Guido Trentalancia wrote:
> > Hello Stephen.
> > 
> > Usually, when a lock file is released, the corresponding file is
> > removed from the filesystem for keeping it clean and tidy.
> > 
> > I might be wrong... But why not ?
> > 
> > If nothing is handling the semanage store, then there shouldn't be a
> > reason for keeping it locked. The presence of a lock file, usually
> > means that the lock is active.
> 
> libsemanage doesn't use the lock files that way; it just uses them as
> the object for flock() operations.  So the presence of the lock file
> means nothing.  Removing it just means it will have to be re-created on
> the next operation.  Not fundamentally opposed, but someone would need
> to validate that it doesn't cause any issues.  It's been that way
> forever.  Maybe the original Tresys authors of this code have an
> opinion on it.

Re: [PATCH] libsemanage: remove lock files

[Alan Jenkins]

On 20/04/17 15:38, Guido Trentalancia wrote:
> Remove semanage read and transaction lock files upon releasing
> them.

What prevents this sequence?

A release lock
  B acquire lock
A unlink lock file
   C create lock file
   C acquire lock

> Signed-off-by: Guido Trentalancia <guido-D1bseh+SzQhuxeB9wqlrNw@public.gmane.org>
> ---
>   src/semanage_store.c |    2 ++
>   1 file changed, 2 insertions(+)
> 
> diff -pruN a/src/semanage_store.c b/src/semanage_store.c
> --- a/src/semanage_store.c	2016-10-14 17:31:26.000000000 +0200
> +++ b/src/semanage_store.c	2017-04-03 09:32:24.093627962 +0200
> @@ -1904,6 +1904,7 @@ void semanage_release_trans_lock(semanag
>   		close(sh->u.direct.translock_file_fd);
>   		sh->u.direct.translock_file_fd = -1;
>   	}
> +	unlink(semanage_files[SEMANAGE_TRANS_LOCK]);
>   	errno = errsv;
>   }
>   
> @@ -1917,6 +1918,7 @@ void semanage_release_active_lock(semana
>   		close(sh->u.direct.activelock_file_fd);
>   		sh->u.direct.activelock_file_fd = -1;
>   	}
> +	unlink(semanage_files[SEMANAGE_READ_LOCK]);
>   	errno = errsv;
>   }

Re: [PATCH] libsemanage: remove lock files

[Alan Jenkins]

*expands thread

Sorry, I see this has already been addressed.


On 24/04/17 13:06, Alan Jenkins wrote:
> On 20/04/17 15:38, Guido Trentalancia wrote:
>> Remove semanage read and transaction lock files upon releasing
>> them.
>
> What prevents this sequence?
>
> A release lock
>  B acquire lock
> A unlink lock file
>   C create lock file
>   C acquire lock
>
>> Signed-off-by: Guido Trentalancia 
>> <guido-D1bseh+SzQhuxeB9wqlrNw@public.gmane.org>
>> ---
>>   src/semanage_store.c |    2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff -pruN a/src/semanage_store.c b/src/semanage_store.c
>> --- a/src/semanage_store.c    2016-10-14 17:31:26.000000000 +0200
>> +++ b/src/semanage_store.c    2017-04-03 09:32:24.093627962 +0200
>> @@ -1904,6 +1904,7 @@ void semanage_release_trans_lock(semanag
>>           close(sh->u.direct.translock_file_fd);
>>           sh->u.direct.translock_file_fd = -1;
>>       }
>> +    unlink(semanage_files[SEMANAGE_TRANS_LOCK]);
>>       errno = errsv;
>>   }
>>   @@ -1917,6 +1918,7 @@ void semanage_release_active_lock(semana
>>           close(sh->u.direct.activelock_file_fd);
>>           sh->u.direct.activelock_file_fd = -1;
>>       }
>> +    unlink(semanage_files[SEMANAGE_READ_LOCK]);
>>       errno = errsv;
>>   }
>
>
>

Re: [PATCH] libsemanage: remove lock files

[Guido Trentalancia]

Yes, we already discussed this possibile race condition. 

Usually there is only one system administrator operating on the semanage store, nevertheless it's worth having a robust locking mechanism...

This patch either needs further work to avoid using flock() and instead using a simpler file lock mechanism with the added benefit of having a cleaner filesystem without confusing stale files around or we just drop the patch given it is not essential to keep things working. 

Regards, 

Guido 

On the 24th of April 2017 14:08:22 CEST, Alan Jenkins <alan.christopher.jenkins@gmail.com> wrote:
>*expands thread
>
>Sorry, I see this has already been addressed.
>
>
>On 24/04/17 13:06, Alan Jenkins wrote:
>> On 20/04/17 15:38, Guido Trentalancia wrote:
>>> Remove semanage read and transaction lock files upon releasing
>>> them.
>>
>> What prevents this sequence?
>>
>> A release lock
>>  B acquire lock
>> A unlink lock file
>>   C create lock file
>>   C acquire lock
>>
>>> Signed-off-by: Guido Trentalancia 
>>> <guido-D1bseh+SzQhuxeB9wqlrNw@public.gmane.org>
>>> ---
>>>   src/semanage_store.c |    2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff -pruN a/src/semanage_store.c b/src/semanage_store.c
>>> --- a/src/semanage_store.c    2016-10-14 17:31:26.000000000 +0200
>>> +++ b/src/semanage_store.c    2017-04-03 09:32:24.093627962 +0200
>>> @@ -1904,6 +1904,7 @@ void semanage_release_trans_lock(semanag
>>>           close(sh->u.direct.translock_file_fd);
>>>           sh->u.direct.translock_file_fd = -1;
>>>       }
>>> +    unlink(semanage_files[SEMANAGE_TRANS_LOCK]);
>>>       errno = errsv;
>>>   }
>>>   @@ -1917,6 +1918,7 @@ void semanage_release_active_lock(semana
>>>           close(sh->u.direct.activelock_file_fd);
>>>           sh->u.direct.activelock_file_fd = -1;
>>>       }
>>> +    unlink(semanage_files[SEMANAGE_READ_LOCK]);
>>>       errno = errsv;
>>>   }
>>
>>
>>

Re: [PATCH] libsemanage: remove lock files

[Guido Trentalancia]

Also, another major benefit of not using flock() comes when using NFS (probably a very rare circumstance, but not entirely impossibile).

It is possible to use the presence of a file (with the same name) to indicate an "active" lock: such file should store the PID of the process that is requiring the lock.

If a lock is found with a PID that does not exist, then such lock is considered invalid and it is removed. 
That is it really...

Regards, 

Guido 

On the 24th of April 2017 19:51:27 CEST, Guido Trentalancia <guido@trentalancia.net> wrote:
>Yes, we already discussed this possibile race condition. 
>
>Usually there is only one system administrator operating on the
>semanage store, nevertheless it's worth having a robust locking
>mechanism...
>
>This patch either needs further work to avoid using flock() and instead
>using a simpler file lock mechanism with the added benefit of having a
>cleaner filesystem without confusing stale files around or we just drop
>the patch given it is not essential to keep things working. 
>
>Regards, 
>
>Guido 
>
>On the 24th of April 2017 14:08:22 CEST, Alan Jenkins
><alan.christopher.jenkins@gmail.com> wrote:
>>*expands thread
>>
>>Sorry, I see this has already been addressed.
>>
>>
>>On 24/04/17 13:06, Alan Jenkins wrote:
>>> On 20/04/17 15:38, Guido Trentalancia wrote:
>>>> Remove semanage read and transaction lock files upon releasing
>>>> them.
>>>
>>> What prevents this sequence?
>>>
>>> A release lock
>>>  B acquire lock
>>> A unlink lock file
>>>   C create lock file
>>>   C acquire lock
>>>
>>>> Signed-off-by: Guido Trentalancia 
>>>> <guido-D1bseh+SzQhuxeB9wqlrNw@public.gmane.org>
>>>> ---
>>>>   src/semanage_store.c |    2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff -pruN a/src/semanage_store.c b/src/semanage_store.c
>>>> --- a/src/semanage_store.c    2016-10-14 17:31:26.000000000 +0200
>>>> +++ b/src/semanage_store.c    2017-04-03 09:32:24.093627962 +0200
>>>> @@ -1904,6 +1904,7 @@ void semanage_release_trans_lock(semanag
>>>>           close(sh->u.direct.translock_file_fd);
>>>>           sh->u.direct.translock_file_fd = -1;
>>>>       }
>>>> +    unlink(semanage_files[SEMANAGE_TRANS_LOCK]);
>>>>       errno = errsv;
>>>>   }
>>>>   @@ -1917,6 +1918,7 @@ void semanage_release_active_lock(semana
>>>>           close(sh->u.direct.activelock_file_fd);
>>>>           sh->u.direct.activelock_file_fd = -1;
>>>>       }
>>>> +    unlink(semanage_files[SEMANAGE_READ_LOCK]);
>>>>       errno = errsv;
>>>>   }
>>>
>>>
>>>

Re: [PATCH] libsemanage: remove lock files

[Russell Coker]

On Tue, 25 Apr 2017 04:38:40 AM Guido Trentalancia wrote:
> Also, another major benefit of not using flock() comes when using NFS
> (probably a very rare circumstance, but not entirely impossibile).
> 
> It is possible to use the presence of a file (with the same name) to
> indicate an "active" lock: such file should store the PID of the process
> that is requiring the lock.
> 
> If a lock is found with a PID that does not exist, then such lock is
> considered invalid and it is removed.  That is it really...

Pidfile locking doesn't work well as pids are not unique, you can have a 
process die and be replaced by another process with the same pid.  Also a 
reboot is expected to have pid conflicts as pids are allocated sequentially and 
most daemons end up with low numbers.  Using a tmpfs for /run solves some of 
these problems as it's reliably cleared out at boot.

Things get even more exciting if you use systemd-nspawn and have multiple pid 
namespaces on the same system with bind mounts of directories that have 
pidfiles.

Pidfile locking also never works across network filesystems as pids are local to 
a system.  You could have some combination of pid and hostname (as done by 
some web browsers) but that gets ugly.

Really pidfiles are so horrible that one of the noteworthy features of systemd 
is removing the need for them.

Having multiple systems operate with NFS root and a shared /etc/selinux is 
never going to work well.  Even if everything works well (and it probably 
won't) you will end up with systems that have the policy in /etc/selinux not 
matching what is running.

-- 
My Main Blog         http://etbe.coker.com.au/
My Documents Blog    http://doc.coker.com.au/

[PATCH v2] libsemanage: remove lock files

[Guido Trentalancia]

Hello.

What follows is a reply to Russell comments and a new, hopefully
better, version of the original patch.

On Tue, 25/04/2017 at 16.30 +1000, Russell Coker wrote:
> On Tue, 25 Apr 2017 04:38:40 AM Guido Trentalancia wrote:
> > Also, another major benefit of not using flock() comes when using
> NFS
> > (probably a very rare circumstance, but not entirely impossibile).
> > 
> > It is possible to use the presence of a file (with the same name)
> to
> > indicate an "active" lock: such file should store the PID of the
> process
> > that is requiring the lock.
> > 
> > If a lock is found with a PID that does not exist, then such lock
> is
> > considered invalid and it is removed.  That is it really...
> 
> Pidfile locking doesn't work well as pids are not unique, you can
> have a 
> process die and be replaced by another process with the same pid. 
> Also a 
> reboot is expected to have pid conflicts as pids are allocated
> sequentially and 
> most daemons end up with low numbers.  Using a tmpfs for /run solves
> some of 
> these problems as it's reliably cleared out at boot.
> 
> Things get even more exciting if you use systemd-nspawn and have
> multiple pid 
> namespaces on the same system with bind mounts of directories that
> have 
> pidfiles.
> 
> Pidfile locking also never works across network filesystems as pids
> are local to 
> a system.  You could have some combination of pid and hostname (as
> done by 
> some web browsers) but that gets ugly.

No, I didn't mean the complicate circumstance of NFS shared amongst
multiple systems.

I only meant the simpler (and most common) situation where the NFS is
mounted for only one system, therefore PIDs are unique and there is no
need to include the hostname.

> Really pidfiles are so horrible that one of the noteworthy features
> of systemd 
> is removing the need for them.

I am not that pessimistic about locking with aid of PIDs.

To be honest, the current situation has more drawbacks in my opinion.

In particular, I don't like the fact that it leaves unused lock files
around the filesystem.

> Having multiple systems operate with NFS root and a shared
> /etc/selinux is 
> never going to work well.  Even if everything works well (and it
> probably 
> won't) you will end up with systems that have the policy in
> /etc/selinux not 
> matching what is running.

Please forget sharing NFS. I meant dedicated NFS.

Anyway, I have created a new version of the patch that should probably
improve the previous race condition.

If you have a way of testing a dedicated store over NFS, then I'd like
to hear back from you the result of such testing !

But, if you are not interested in testing it, then never mind...

---
Do not use flock() for file locking, but instead use generic text files
that keep track of the process ID (PID) of the locking process.

Remove semanage read and transaction lock files upon releasing
them.

Signed-off-by: Guido Trentalancia <guido@trentalancia.net>
---
 src/Makefile         |    2
 src/semanage_store.c |  205 ++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 154 insertions(+), 53 deletions(-)

--- a/src/Makefile	2016-10-14 17:31:26.000000000 +0200
+++ b/src/Makefile	2017-04-25 00:54:58.916827639 +0200
@@ -91,7 +91,7 @@ $(LIBA): $(OBJS)
 	$(RANLIB) $@
 
 $(LIBSO): $(LOBJS)
-	$(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lsepol -laudit -lselinux -lbz2 -lustr -L$(LIBDIR) -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs
+	$(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lm -lsepol -laudit -lselinux -lbz2 -lustr -L$(LIBDIR) -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs
 	ln -sf $@ $(TARGET)
 
 $(LIBPC): $(LIBPC).in ../VERSION
--- a/src/semanage_store.c	2016-10-14 17:31:26.000000000 +0200
+++ b/src/semanage_store.c	2017-04-25 21:52:51.977563955 +0200
@@ -45,6 +45,8 @@ typedef struct dbase_policydb dbase_t;
 #include <dirent.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <math.h>
+#include <signal.h>
 #include <stdio.h>
 #include <stdio_ext.h>
 #include <stdlib.h>
@@ -52,11 +54,24 @@ typedef struct dbase_policydb dbase_t;
 #include <unistd.h>
 #include <sys/file.h>
 #include <sys/stat.h>
+#include <sys/sysctl.h>
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <limits.h>
 #include <libgen.h>
 
+#include <linux/sysctl.h>
+
+#ifndef CONFIG_BASE_SMALL
+#define CONFIG_BASE_SMALL       0
+#endif
+
+#include <linux/threads.h>
+
+#ifndef PID_MAX_DEFAULT
+#define PID_MAX_DEFAULT 32768
+#endif
+
 #include "debug.h"
 #include "utilities.h"
 
@@ -76,6 +91,8 @@ enum semanage_file_defs {
 static char *semanage_paths[SEMANAGE_NUM_STORES][SEMANAGE_STORE_NUM_PATHS];
 static char *semanage_files[SEMANAGE_NUM_FILES] = { NULL };
 static int semanage_paths_initialized = 0;
+static int pid_max;
+static ssize_t pid_max_length;
 
 /* These are paths relative to the bottom of the module store */
 static const char *semanage_relative_files[SEMANAGE_NUM_FILES] = {
@@ -425,8 +442,23 @@ cleanup:
 int semanage_check_init(semanage_handle_t *sh, const char *prefix)
 {
 	int rc;
+	int fd;
+	char root[PATH_MAX];
+	ssize_t amount_read;
+
 	if (semanage_paths_initialized == 0) {
-		char root[PATH_MAX];
+		pid_max = PID_MAX_DEFAULT;
+		pid_max_length = ceil(log10(PID_MAX_DEFAULT + 1));
+
+		fd = open("/proc/sys/kernel/pid_max", O_RDONLY);
+		if (fd > 0) {
+			char sysctlstring[pid_max_length];
+			amount_read = read(fd, sysctlstring, pid_max_length);
+			if (amount_read > 0) {
+				pid_max = atoi(sysctlstring);
+				pid_max_length = ceil(log10(pid_max + 1));
+			}
+		}
 
 		rc = snprintf(root,
 			      sizeof(root),
@@ -526,16 +558,23 @@ char *semanage_conf_path(void)
 
 /**************** functions that create module store ***************/
 
-/* Check that the semanage store exists.  If 'create' is non-zero then
- * create the directories.  Returns 0 if module store exists (either
- * already or just created), -1 if does not exist or could not be
- * read, or -2 if it could not create the store. */
+/* Check that the semanage store exists and that the read lock can be
+ * taken.  If 'create' is non-zero then it creates the directories
+ * and the lock file.  Returns 0 if the module store exists (either
+ * already or just created) and the read lock can be taken, -1 if it
+ * does not exist or it is not possible to read from it, or -2 if it
+ * could not create the store or it could not take the lock file. */
 int semanage_create_store(semanage_handle_t * sh, int create)
 {
 	struct stat sb;
 	int mode_mask = R_OK | W_OK | X_OK;
 	const char *path = semanage_files[SEMANAGE_ROOT];
 	int fd;
+	pid_t pid, lock_pid;
+	char *pid_string, *lock_pid_string;
+	size_t pid_length;
+	ssize_t pid_bytes;
+	int invalid_lock = 0;
 
 	if (stat(path, &sb) == -1) {
 		if (errno == ENOENT && create) {
@@ -605,26 +644,82 @@ int semanage_create_store(semanage_handl
 			return -1;
 		}
 	}
+	pid = getpid();
+	pid_string = malloc(pid_max_length * sizeof(char));
+	sprintf(pid_string, "%d", pid);
+	pid_length = strlen(pid_string);
 	path = semanage_files[SEMANAGE_READ_LOCK];
 	if (stat(path, &sb) == -1) {
 		if (errno == ENOENT && create) {
 			if ((fd = creat(path, S_IRUSR | S_IWUSR)) == -1) {
 				ERR(sh, "Could not create lock file at %s.",
 				    path);
+				free(pid_string);
 				return -2;
 			}
+			pid_bytes = write(fd, pid_string, pid_length);
 			close(fd);
+			free(pid_string);
+			if (pid_bytes == (ssize_t) pid_length)
+				return 0;
+			else {
+				ERR(sh, "Could not create lock file at %s.", path);
+				return -2;
+			}
 		} else {
 			ERR(sh, "Could not read lock file at %s.", path);
+			free(pid_string);
 			return -1;
 		}
 	} else {
 		if (!S_ISREG(sb.st_mode) || access(path, R_OK | W_OK) == -1) {
 			ERR(sh, "Could not access lock file at %s.", path);
+			free(pid_string);
 			return -1;
-		}
+		} else {
+			fd = open(path, O_RDWR);
+			if (fd > 0) {
+				lock_pid_string = malloc(pid_max_length * sizeof(char));
+				pid_bytes = read(fd, lock_pid_string, pid_max_length);
+				if (pid_bytes > 0) {
+					lock_pid = (pid_t) atoi(lock_pid_string);
+					if (lock_pid > pid_max)
+						invalid_lock = 1;
+					else if (kill(lock_pid, 0) != 0)
+						if (errno == ESRCH)
+							invalid_lock = 1;
+
+					if (!invalid_lock) {
+						ERR(sh, "Could not create lock file at %s.", path);
+						close(fd);
+						free(pid_string);
+						free(lock_pid_string);
+						return -2;
+					}
+				} else
+					invalid_lock = 1;
+
+				if (invalid_lock) {
+					pid_bytes = pwrite(fd, pid_string, pid_length, 0);
+					close(fd);
+					free(pid_string);
+					free(lock_pid_string);
+					if (pid_bytes == (ssize_t) pid_length)
+						return 0;
+					else {
+						ERR(sh, "Could not create lock file at %s.", path);
+						return -2;
+					}
+				} else {
+					ERR(sh, "Could not create lock file at %s.", path);
+					close(fd);
+					free(pid_string);
+					free(lock_pid_string);
+					return -2;
+				}
+			}
+		}	
 	}
-	return 0;
 }
 
 /* returns <0 if the active store cannot be read or doesn't exist
@@ -1788,64 +1883,74 @@ int semanage_install_sandbox(semanage_ha
 static int semanage_get_lock(semanage_handle_t * sh,
 			     const char *lock_name, const char *lock_file)
 {
+	pid_t pid, lock_pid;
+	char *pid_string, *lock_pid_string;
 	int fd;
-	struct timeval origtime, curtime;
+	size_t pid_length;
+	ssize_t pid_bytes;
 	int got_lock = 0;
+	int overwritten_lock = 0;
 
-	if ((fd = open(lock_file, O_RDONLY)) == -1) {
-		if ((fd =
-		     open(lock_file, O_RDWR | O_CREAT | O_TRUNC,
-			  S_IRUSR | S_IWUSR)) == -1) {
+	pid = getpid();
+	pid_string = malloc(pid_max_length * sizeof(char));
+	sprintf(pid_string, "%d", pid);
+	pid_length = strlen(pid_string);
+	if ((fd = open(lock_file, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR)) == -1) {
 			ERR(sh, "Could not open direct %s at %s.", lock_name,
 			    lock_file);
+			free(pid_string);
 			return -1;
+	} else {
+		lock_pid_string = malloc(pid_max_length * sizeof(char));
+		pid_bytes = pread(fd, lock_pid_string, pid_max_length, 0);
+		if (pid_bytes == 0) {
+			overwritten_lock = 1;
+			pid_bytes = write(fd, pid_string, pid_length);
+			if (pid_bytes == (ssize_t) pid_length)
+				got_lock = 1;
+		} else {
+			lock_pid = (pid_t) atoi(lock_pid_string);
+			if (lock_pid > pid_max) {
+				overwritten_lock = 1;
+				pid_bytes = pwrite(fd, pid_string, pid_length, 0);
+				if (pid_bytes == (ssize_t) pid_length)
+					got_lock = 1;
+			} else {
+				if (lock_pid = pid)
+				       got_lock = 1;
+				else if (kill(lock_pid, 0) != 0)
+					if (errno == ESRCH) {
+						overwritten_lock = 1;
+						pid_bytes = pwrite(fd, pid_string, pid_length, 0);
+						if (pid_bytes == (ssize_t) pid_length)
+							got_lock = 1;
+					}
+			}
 		}
 	}
-	if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) {
-		ERR(sh, "Could not set close-on-exec for %s at %s.", lock_name,
-		    lock_file);
+
+	if (!got_lock) {
 		close(fd);
+		free(pid_string);
+		free(lock_pid_string);
+		if (overwritten_lock)
+			unlink(lock_file);
+		ERR(sh, "Could not get direct %s at %s.", lock_name, lock_file);
 		return -1;
 	}
 
-	if (sh->timeout == 0) {
-		/* return immediately */
-		origtime.tv_sec = 0;
-	} else {
-		origtime.tv_sec = sh->timeout;
-	}
-	origtime.tv_usec = 0;
-	do {
-		curtime.tv_sec = 1;
-		curtime.tv_usec = 0;
-		if (flock(fd, LOCK_EX | LOCK_NB) == 0) {
-			got_lock = 1;
-			break;
-		} else if (errno != EAGAIN) {
-			ERR(sh, "Error obtaining direct %s at %s.", lock_name,
-			    lock_file);
-			close(fd);
-			return -1;
-		}
-		if (origtime.tv_sec > 0 || sh->timeout == -1) {
-			if (select(0, NULL, NULL, NULL, &curtime) == -1) {
-				if (errno == EINTR) {
-					continue;
-				}
-				ERR(sh,
-				    "Error while waiting to get direct %s at %s.",
-				    lock_name, lock_file);
-				close(fd);
-				return -1;
-			}
-			origtime.tv_sec--;
-		}
-	} while (origtime.tv_sec > 0 || sh->timeout == -1);
-	if (!got_lock) {
-		ERR(sh, "Could not get direct %s at %s.", lock_name, lock_file);
+	if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) {
+		ERR(sh, "Could not set close-on-exec for %s at %s.", lock_name,
+		    lock_file);
 		close(fd);
+		free(pid_string);
+		free(lock_pid_string);
+		unlink(lock_file);
 		return -1;
 	}
+
+	free(pid_string);
+	free(lock_pid_string);
 	return fd;
 }
 
@@ -1894,29 +1999,27 @@ int semanage_get_active_lock(semanage_ha
 	}
 }
 
-/* Releases the transaction lock.  Does nothing if there was not one already
- * there. */
+/* Releases the transaction lock: the lock file is removed */
 void semanage_release_trans_lock(semanage_handle_t * sh)
 {
 	int errsv = errno;
 	if (sh->u.direct.translock_file_fd >= 0) {
-		flock(sh->u.direct.translock_file_fd, LOCK_UN);
 		close(sh->u.direct.translock_file_fd);
 		sh->u.direct.translock_file_fd = -1;
 	}
+	unlink(semanage_files[SEMANAGE_TRANS_LOCK]);
 	errno = errsv;
 }
 
-/* Releases the read lock.  Does nothing if there was not one already
- * there. */
+/* Releases the read lock: the lock file is removed */
 void semanage_release_active_lock(semanage_handle_t * sh)
 {
 	int errsv = errno;
 	if (sh->u.direct.activelock_file_fd >= 0) {
-		flock(sh->u.direct.activelock_file_fd, LOCK_UN);
 		close(sh->u.direct.activelock_file_fd);
 		sh->u.direct.activelock_file_fd = -1;
 	}
+	unlink(semanage_files[SEMANAGE_READ_LOCK]);
 	errno = errsv;
 }
 

[PATCH v3] libsemanage: remove lock files

[Guido Trentalancia]

Do not use flock() for file locking, but instead use generic text files
that keep track of the process ID (PID) of the locking process.

Remove semanage read and transaction lock files upon releasing
them.

This third version fixes a bug in the previous version and also applies
cleanly to the latest git tree.

Signed-off-by: Guido Trentalancia <guido@trentalancia.net>
---
 src/Makefile         |    2
 src/semanage_store.c |  214 +++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 160 insertions(+), 56 deletions(-)

--- a/src/Makefile	2017-04-25 22:27:38.105555427 +0200
+++ b/src/Makefile	2017-04-25 22:28:58.512555098 +0200
@@ -91,7 +91,7 @@ $(LIBA): $(OBJS)
 	$(RANLIB) $@
 
 $(LIBSO): $(LOBJS)
-	$(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lsepol -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs
+	$(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lm -lsepol -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs
 	ln -sf $@ $(TARGET)
 
 $(LIBPC): $(LIBPC).in ../VERSION
--- a/src/semanage_store.c	2017-04-20 16:30:21.218209972 +0200
+++ b/src/semanage_store.c	2017-04-25 22:24:35.883556172 +0200
@@ -45,6 +45,8 @@ typedef struct dbase_policydb dbase_t;
 #include <dirent.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <math.h>
+#include <signal.h>
 #include <stdio.h>
 #include <stdio_ext.h>
 #include <stdlib.h>
@@ -52,11 +54,24 @@ typedef struct dbase_policydb dbase_t;
 #include <unistd.h>
 #include <sys/file.h>
 #include <sys/stat.h>
+#include <sys/sysctl.h>
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <limits.h>
 #include <libgen.h>
 
+#include <linux/sysctl.h>
+
+#ifndef CONFIG_BASE_SMALL
+#define CONFIG_BASE_SMALL       0
+#endif
+
+#include <linux/threads.h>
+
+#ifndef PID_MAX_DEFAULT
+#define PID_MAX_DEFAULT 32768
+#endif
+
 #include "debug.h"
 #include "utilities.h"
 
@@ -76,6 +91,8 @@ enum semanage_file_defs {
 static char *semanage_paths[SEMANAGE_NUM_STORES][SEMANAGE_STORE_NUM_PATHS];
 static char *semanage_files[SEMANAGE_NUM_FILES] = { NULL };
 static int semanage_paths_initialized = 0;
+static int pid_max;
+static ssize_t pid_max_length;
 
 /* These are paths relative to the bottom of the module store */
 static const char *semanage_relative_files[SEMANAGE_NUM_FILES] = {
@@ -427,8 +442,23 @@ cleanup:
 int semanage_check_init(semanage_handle_t *sh, const char *prefix)
 {
 	int rc;
+	int fd;
+	char root[PATH_MAX];
+	ssize_t amount_read;
+
 	if (semanage_paths_initialized == 0) {
-		char root[PATH_MAX];
+		pid_max = PID_MAX_DEFAULT;
+		pid_max_length = ceil(log10(PID_MAX_DEFAULT + 1));
+
+		fd = open("/proc/sys/kernel/pid_max", O_RDONLY);
+		if (fd > 0) {
+			char sysctlstring[pid_max_length];
+			amount_read = read(fd, sysctlstring, pid_max_length);
+			if (amount_read > 0) {
+				pid_max = atoi(sysctlstring);
+				pid_max_length = ceil(log10(pid_max + 1));
+			}
+		}
 
 		rc = snprintf(root,
 			      sizeof(root),
@@ -528,16 +558,23 @@ char *semanage_conf_path(void)
 
 /**************** functions that create module store ***************/
 
-/* Check that the semanage store exists.  If 'create' is non-zero then
- * create the directories.  Returns 0 if module store exists (either
- * already or just created), -1 if does not exist or could not be
- * read, or -2 if it could not create the store. */
+/* Check that the semanage store exists and that the read lock can be
+ * taken.  If 'create' is non-zero then it creates the directories
+ * and the lock file.  Returns 0 if the module store exists (either
+ * already or just created) and the read lock can be taken, -1 if it
+ * does not exist or it is not possible to read from it, or -2 if it
+ * could not create the store or it could not take the lock file. */
 int semanage_create_store(semanage_handle_t * sh, int create)
 {
 	struct stat sb;
 	int mode_mask = R_OK | W_OK | X_OK;
 	const char *path = semanage_files[SEMANAGE_ROOT];
 	int fd;
+	pid_t pid, lock_pid;
+	char *pid_string, *lock_pid_string;
+	size_t pid_length;
+	ssize_t pid_bytes;
+	int invalid_lock = 0;
 
 	if (stat(path, &sb) == -1) {
 		if (errno == ENOENT && create) {
@@ -607,24 +644,81 @@ int semanage_create_store(semanage_handl
 			return -1;
 		}
 	}
+	pid = getpid();
+	pid_string = malloc(pid_max_length * sizeof(char));
+	sprintf(pid_string, "%d", pid);
+	pid_length = strlen(pid_string);
 	path = semanage_files[SEMANAGE_READ_LOCK];
 	if (stat(path, &sb) == -1) {
 		if (errno == ENOENT && create) {
 			if ((fd = creat(path, S_IRUSR | S_IWUSR)) == -1) {
 				ERR(sh, "Could not create lock file at %s.",
 				    path);
+				free(pid_string);
 				return -2;
 			}
+			pid_bytes = write(fd, pid_string, pid_length);
 			close(fd);
+			free(pid_string);
+			if (pid_bytes == (ssize_t) pid_length)
+				return 0;
+			else {
+				ERR(sh, "Could not create lock file at %s.", path);
+				return -2;
+			}
 		} else {
 			ERR(sh, "Could not read lock file at %s.", path);
+			free(pid_string);
 			return -1;
 		}
 	} else {
 		if (!S_ISREG(sb.st_mode) || access(path, R_OK | W_OK) == -1) {
 			ERR(sh, "Could not access lock file at %s.", path);
+			free(pid_string);
 			return -1;
-		}
+		} else {
+			fd = open(path, O_RDWR);
+			if (fd > 0) {
+				lock_pid_string = malloc(pid_max_length * sizeof(char));
+				pid_bytes = read(fd, lock_pid_string, pid_max_length);
+				if (pid_bytes > 0) {
+					lock_pid = (pid_t) atoi(lock_pid_string);
+					if (lock_pid > pid_max)
+						invalid_lock = 1;
+					else if (kill(lock_pid, 0) != 0)
+						if (errno == ESRCH)
+							invalid_lock = 1;
+
+					if (!invalid_lock) {
+						ERR(sh, "Could not create lock file at %s.", path);
+						close(fd);
+						free(pid_string);
+						free(lock_pid_string);
+						return -2;
+					}
+				} else
+					invalid_lock = 1;
+
+				if (invalid_lock) {
+					pid_bytes = pwrite(fd, pid_string, pid_length, 0);
+					close(fd);
+					free(pid_string);
+					free(lock_pid_string);
+					if (pid_bytes == (ssize_t) pid_length)
+						return 0;
+					else {
+						ERR(sh, "Could not create lock file at %s.", path);
+						return -2;
+					}
+				} else {
+					ERR(sh, "Could not create lock file at %s.", path);
+					close(fd);
+					free(pid_string);
+					free(lock_pid_string);
+					return -2;
+				}
+			}
+		}	
 	}
 	return 0;
 }
@@ -1795,64 +1884,74 @@ int semanage_install_sandbox(semanage_ha
 static int semanage_get_lock(semanage_handle_t * sh,
 			     const char *lock_name, const char *lock_file)
 {
+	pid_t pid, lock_pid;
+	char *pid_string, *lock_pid_string;
 	int fd;
-	struct timeval origtime, curtime;
+	size_t pid_length;
+	ssize_t pid_bytes;
 	int got_lock = 0;
+	int overwritten_lock = 0;
 
-	if ((fd = open(lock_file, O_RDONLY)) == -1) {
-		if ((fd =
-		     open(lock_file, O_RDWR | O_CREAT | O_TRUNC,
-			  S_IRUSR | S_IWUSR)) == -1) {
+	pid = getpid();
+	pid_string = malloc(pid_max_length * sizeof(char));
+	sprintf(pid_string, "%d", pid);
+	pid_length = strlen(pid_string);
+	if ((fd = open(lock_file, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR)) == -1) {
 			ERR(sh, "Could not open direct %s at %s.", lock_name,
 			    lock_file);
+			free(pid_string);
 			return -1;
+	} else {
+		lock_pid_string = malloc(pid_max_length * sizeof(char));
+		pid_bytes = pread(fd, lock_pid_string, pid_max_length, 0);
+		if (pid_bytes == 0) {
+			overwritten_lock = 1;
+			pid_bytes = write(fd, pid_string, pid_length);
+			if (pid_bytes == (ssize_t) pid_length)
+				got_lock = 1;
+		} else {
+			lock_pid = (pid_t) atoi(lock_pid_string);
+			if (lock_pid > pid_max) {
+				overwritten_lock = 1;
+				pid_bytes = pwrite(fd, pid_string, pid_length, 0);
+				if (pid_bytes == (ssize_t) pid_length)
+					got_lock = 1;
+			} else {
+				if (lock_pid == pid)
+				       got_lock = 1;
+				else if (kill(lock_pid, 0) != 0)
+					if (errno == ESRCH) {
+						overwritten_lock = 1;
+						pid_bytes = pwrite(fd, pid_string, pid_length, 0);
+						if (pid_bytes == (ssize_t) pid_length)
+							got_lock = 1;
+					}
+			}
 		}
 	}
-	if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) {
-		ERR(sh, "Could not set close-on-exec for %s at %s.", lock_name,
-		    lock_file);
+
+	if (!got_lock) {
 		close(fd);
+		free(pid_string);
+		free(lock_pid_string);
+		if (overwritten_lock)
+			unlink(lock_file);
+		ERR(sh, "Could not get direct %s at %s.", lock_name, lock_file);
 		return -1;
 	}
 
-	if (sh->timeout == 0) {
-		/* return immediately */
-		origtime.tv_sec = 0;
-	} else {
-		origtime.tv_sec = sh->timeout;
-	}
-	origtime.tv_usec = 0;
-	do {
-		curtime.tv_sec = 1;
-		curtime.tv_usec = 0;
-		if (flock(fd, LOCK_EX | LOCK_NB) == 0) {
-			got_lock = 1;
-			break;
-		} else if (errno != EAGAIN) {
-			ERR(sh, "Error obtaining direct %s at %s.", lock_name,
-			    lock_file);
-			close(fd);
-			return -1;
-		}
-		if (origtime.tv_sec > 0 || sh->timeout == -1) {
-			if (select(0, NULL, NULL, NULL, &curtime) == -1) {
-				if (errno == EINTR) {
-					continue;
-				}
-				ERR(sh,
-				    "Error while waiting to get direct %s at %s.",
-				    lock_name, lock_file);
-				close(fd);
-				return -1;
-			}
-			origtime.tv_sec--;
-		}
-	} while (origtime.tv_sec > 0 || sh->timeout == -1);
-	if (!got_lock) {
-		ERR(sh, "Could not get direct %s at %s.", lock_name, lock_file);
+	if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) {
+		ERR(sh, "Could not set close-on-exec for %s at %s.", lock_name,
+		    lock_file);
 		close(fd);
+		free(pid_string);
+		free(lock_pid_string);
+		unlink(lock_file);
 		return -1;
 	}
+
+	free(pid_string);
+	free(lock_pid_string);
 	return fd;
 }
 
@@ -1901,29 +2000,27 @@ int semanage_get_active_lock(semanage_ha
 	}
 }
 
-/* Releases the transaction lock.  Does nothing if there was not one already
- * there. */
+/* Releases the transaction lock: the lock file is removed */
 void semanage_release_trans_lock(semanage_handle_t * sh)
 {
 	int errsv = errno;
 	if (sh->u.direct.translock_file_fd >= 0) {
-		flock(sh->u.direct.translock_file_fd, LOCK_UN);
 		close(sh->u.direct.translock_file_fd);
 		sh->u.direct.translock_file_fd = -1;
 	}
+	unlink(semanage_files[SEMANAGE_TRANS_LOCK]);
 	errno = errsv;
 }
 
-/* Releases the read lock.  Does nothing if there was not one already
- * there. */
+/* Releases the read lock: the lock file is removed */
 void semanage_release_active_lock(semanage_handle_t * sh)
 {
 	int errsv = errno;
 	if (sh->u.direct.activelock_file_fd >= 0) {
-		flock(sh->u.direct.activelock_file_fd, LOCK_UN);
 		close(sh->u.direct.activelock_file_fd);
 		sh->u.direct.activelock_file_fd = -1;
 	}
+	unlink(semanage_files[SEMANAGE_READ_LOCK]);
 	errno = errsv;
 }
 

Re: [PATCH v3] libsemanage: remove lock files

[Jason Zaman]

On Tue, Apr 25, 2017 at 10:35:17PM +0200, Guido Trentalancia wrote:
> Do not use flock() for file locking, but instead use generic text files
> that keep track of the process ID (PID) of the locking process.
> 
> Remove semanage read and transaction lock files upon releasing
> them.
> 
> This third version fixes a bug in the previous version and also applies
> cleanly to the latest git tree.
> 
> Signed-off-by: Guido Trentalancia <guido@trentalancia.net>
> ---
>  src/Makefile         |    2
>  src/semanage_store.c |  214 +++++++++++++++++++++++++++++++++++++--------------
>  2 files changed, 160 insertions(+), 56 deletions(-)
> 
> --- a/src/Makefile	2017-04-25 22:27:38.105555427 +0200
> +++ b/src/Makefile	2017-04-25 22:28:58.512555098 +0200
> @@ -91,7 +91,7 @@ $(LIBA): $(OBJS)
>  	$(RANLIB) $@
>  
>  $(LIBSO): $(LOBJS)
> -	$(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lsepol -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs
> +	$(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lm -lsepol -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs
>  	ln -sf $@ $(TARGET)
>  
>  $(LIBPC): $(LIBPC).in ../VERSION
> --- a/src/semanage_store.c	2017-04-20 16:30:21.218209972 +0200
> +++ b/src/semanage_store.c	2017-04-25 22:24:35.883556172 +0200
> @@ -45,6 +45,8 @@ typedef struct dbase_policydb dbase_t;
>  #include <dirent.h>
>  #include <errno.h>
>  #include <fcntl.h>
> +#include <math.h>
> +#include <signal.h>
>  #include <stdio.h>
>  #include <stdio_ext.h>
>  #include <stdlib.h>
> @@ -52,11 +54,24 @@ typedef struct dbase_policydb dbase_t;
>  #include <unistd.h>
>  #include <sys/file.h>
>  #include <sys/stat.h>
> +#include <sys/sysctl.h>
>  #include <sys/types.h>
>  #include <sys/wait.h>
>  #include <limits.h>
>  #include <libgen.h>
>  
> +#include <linux/sysctl.h>
> +
> +#ifndef CONFIG_BASE_SMALL
> +#define CONFIG_BASE_SMALL       0
> +#endif
> +
> +#include <linux/threads.h>
> +
> +#ifndef PID_MAX_DEFAULT
> +#define PID_MAX_DEFAULT 32768
> +#endif
> +
>  #include "debug.h"
>  #include "utilities.h"
>  
> @@ -76,6 +91,8 @@ enum semanage_file_defs {
>  static char *semanage_paths[SEMANAGE_NUM_STORES][SEMANAGE_STORE_NUM_PATHS];
>  static char *semanage_files[SEMANAGE_NUM_FILES] = { NULL };
>  static int semanage_paths_initialized = 0;
> +static int pid_max;
> +static ssize_t pid_max_length;
>  
>  /* These are paths relative to the bottom of the module store */
>  static const char *semanage_relative_files[SEMANAGE_NUM_FILES] = {
> @@ -427,8 +442,23 @@ cleanup:
>  int semanage_check_init(semanage_handle_t *sh, const char *prefix)
>  {
>  	int rc;
> +	int fd;
> +	char root[PATH_MAX];
> +	ssize_t amount_read;
> +
>  	if (semanage_paths_initialized == 0) {
> -		char root[PATH_MAX];
> +		pid_max = PID_MAX_DEFAULT;
> +		pid_max_length = ceil(log10(PID_MAX_DEFAULT + 1));
> +
> +		fd = open("/proc/sys/kernel/pid_max", O_RDONLY);
> +		if (fd > 0) {
> +			char sysctlstring[pid_max_length];
> +			amount_read = read(fd, sysctlstring, pid_max_length);
> +			if (amount_read > 0) {
> +				pid_max = atoi(sysctlstring);
> +				pid_max_length = ceil(log10(pid_max + 1));
> +			}
> +		}
>  
>  		rc = snprintf(root,
>  			      sizeof(root),
> @@ -528,16 +558,23 @@ char *semanage_conf_path(void)
>  
>  /**************** functions that create module store ***************/
>  
> -/* Check that the semanage store exists.  If 'create' is non-zero then
> - * create the directories.  Returns 0 if module store exists (either
> - * already or just created), -1 if does not exist or could not be
> - * read, or -2 if it could not create the store. */
> +/* Check that the semanage store exists and that the read lock can be
> + * taken.  If 'create' is non-zero then it creates the directories
> + * and the lock file.  Returns 0 if the module store exists (either
> + * already or just created) and the read lock can be taken, -1 if it
> + * does not exist or it is not possible to read from it, or -2 if it
> + * could not create the store or it could not take the lock file. */
>  int semanage_create_store(semanage_handle_t * sh, int create)
>  {
>  	struct stat sb;
>  	int mode_mask = R_OK | W_OK | X_OK;
>  	const char *path = semanage_files[SEMANAGE_ROOT];
>  	int fd;
> +	pid_t pid, lock_pid;
> +	char *pid_string, *lock_pid_string;
> +	size_t pid_length;
> +	ssize_t pid_bytes;
> +	int invalid_lock = 0;
>  
>  	if (stat(path, &sb) == -1) {
>  		if (errno == ENOENT && create) {
> @@ -607,24 +644,81 @@ int semanage_create_store(semanage_handl
>  			return -1;
>  		}
>  	}
> +	pid = getpid();
> +	pid_string = malloc(pid_max_length * sizeof(char));
> +	sprintf(pid_string, "%d", pid);
> +	pid_length = strlen(pid_string);
>  	path = semanage_files[SEMANAGE_READ_LOCK];
>  	if (stat(path, &sb) == -1) {
>  		if (errno == ENOENT && create) {
>  			if ((fd = creat(path, S_IRUSR | S_IWUSR)) == -1) {
>  				ERR(sh, "Could not create lock file at %s.",
>  				    path);
> +				free(pid_string);
>  				return -2;
>  			}
> +			pid_bytes = write(fd, pid_string, pid_length);

I'm pretty sure there is a race here. and another one between stat() and
creat(). you can have two processes create and truncate the file then
one write on top of the other. leaving lock files around is definitely
the safest option. and are 0 bytes so its not like you're wasting any
disk space.

Overall I disagree with changing this just because a random file is left
around. NFS may be a legit reason to change but as russel said, I think
locks work somewhat over NFS too. Also, NFS mounting /home or /usr is
common but /var is pretty commonly not NFS mounted.

-- Jason

>  			close(fd);
> +			free(pid_string);
> +			if (pid_bytes == (ssize_t) pid_length)
> +				return 0;
> +			else {
> +				ERR(sh, "Could not create lock file at %s.", path);
> +				return -2;
> +			}
>  		} else {
>  			ERR(sh, "Could not read lock file at %s.", path);
> +			free(pid_string);
>  			return -1;
>  		}
>  	} else {
>  		if (!S_ISREG(sb.st_mode) || access(path, R_OK | W_OK) == -1) {
>  			ERR(sh, "Could not access lock file at %s.", path);
> +			free(pid_string);
>  			return -1;
> -		}
> +		} else {
> +			fd = open(path, O_RDWR);
> +			if (fd > 0) {
> +				lock_pid_string = malloc(pid_max_length * sizeof(char));
> +				pid_bytes = read(fd, lock_pid_string, pid_max_length);
> +				if (pid_bytes > 0) {
> +					lock_pid = (pid_t) atoi(lock_pid_string);
> +					if (lock_pid > pid_max)
> +						invalid_lock = 1;
> +					else if (kill(lock_pid, 0) != 0)
> +						if (errno == ESRCH)
> +							invalid_lock = 1;
> +
> +					if (!invalid_lock) {
> +						ERR(sh, "Could not create lock file at %s.", path);
> +						close(fd);
> +						free(pid_string);
> +						free(lock_pid_string);
> +						return -2;
> +					}
> +				} else
> +					invalid_lock = 1;
> +
> +				if (invalid_lock) {
> +					pid_bytes = pwrite(fd, pid_string, pid_length, 0);
> +					close(fd);
> +					free(pid_string);
> +					free(lock_pid_string);
> +					if (pid_bytes == (ssize_t) pid_length)
> +						return 0;
> +					else {
> +						ERR(sh, "Could not create lock file at %s.", path);
> +						return -2;
> +					}
> +				} else {
> +					ERR(sh, "Could not create lock file at %s.", path);
> +					close(fd);
> +					free(pid_string);
> +					free(lock_pid_string);
> +					return -2;
> +				}
> +			}
> +		}	
>  	}
>  	return 0;
>  }
> @@ -1795,64 +1884,74 @@ int semanage_install_sandbox(semanage_ha
>  static int semanage_get_lock(semanage_handle_t * sh,
>  			     const char *lock_name, const char *lock_file)
>  {
> +	pid_t pid, lock_pid;
> +	char *pid_string, *lock_pid_string;
>  	int fd;
> -	struct timeval origtime, curtime;
> +	size_t pid_length;
> +	ssize_t pid_bytes;
>  	int got_lock = 0;
> +	int overwritten_lock = 0;
>  
> -	if ((fd = open(lock_file, O_RDONLY)) == -1) {
> -		if ((fd =
> -		     open(lock_file, O_RDWR | O_CREAT | O_TRUNC,
> -			  S_IRUSR | S_IWUSR)) == -1) {
> +	pid = getpid();
> +	pid_string = malloc(pid_max_length * sizeof(char));
> +	sprintf(pid_string, "%d", pid);
> +	pid_length = strlen(pid_string);
> +	if ((fd = open(lock_file, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR)) == -1) {
>  			ERR(sh, "Could not open direct %s at %s.", lock_name,
>  			    lock_file);
> +			free(pid_string);
>  			return -1;
> +	} else {
> +		lock_pid_string = malloc(pid_max_length * sizeof(char));
> +		pid_bytes = pread(fd, lock_pid_string, pid_max_length, 0);
> +		if (pid_bytes == 0) {
> +			overwritten_lock = 1;
> +			pid_bytes = write(fd, pid_string, pid_length);
> +			if (pid_bytes == (ssize_t) pid_length)
> +				got_lock = 1;
> +		} else {
> +			lock_pid = (pid_t) atoi(lock_pid_string);
> +			if (lock_pid > pid_max) {
> +				overwritten_lock = 1;
> +				pid_bytes = pwrite(fd, pid_string, pid_length, 0);
> +				if (pid_bytes == (ssize_t) pid_length)
> +					got_lock = 1;
> +			} else {
> +				if (lock_pid == pid)
> +				       got_lock = 1;
> +				else if (kill(lock_pid, 0) != 0)
> +					if (errno == ESRCH) {
> +						overwritten_lock = 1;
> +						pid_bytes = pwrite(fd, pid_string, pid_length, 0);
> +						if (pid_bytes == (ssize_t) pid_length)
> +							got_lock = 1;
> +					}
> +			}
>  		}
>  	}
> -	if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) {
> -		ERR(sh, "Could not set close-on-exec for %s at %s.", lock_name,
> -		    lock_file);
> +
> +	if (!got_lock) {
>  		close(fd);
> +		free(pid_string);
> +		free(lock_pid_string);
> +		if (overwritten_lock)
> +			unlink(lock_file);
> +		ERR(sh, "Could not get direct %s at %s.", lock_name, lock_file);
>  		return -1;
>  	}
>  
> -	if (sh->timeout == 0) {
> -		/* return immediately */
> -		origtime.tv_sec = 0;
> -	} else {
> -		origtime.tv_sec = sh->timeout;
> -	}
> -	origtime.tv_usec = 0;
> -	do {
> -		curtime.tv_sec = 1;
> -		curtime.tv_usec = 0;
> -		if (flock(fd, LOCK_EX | LOCK_NB) == 0) {
> -			got_lock = 1;
> -			break;
> -		} else if (errno != EAGAIN) {
> -			ERR(sh, "Error obtaining direct %s at %s.", lock_name,
> -			    lock_file);
> -			close(fd);
> -			return -1;
> -		}
> -		if (origtime.tv_sec > 0 || sh->timeout == -1) {
> -			if (select(0, NULL, NULL, NULL, &curtime) == -1) {
> -				if (errno == EINTR) {
> -					continue;
> -				}
> -				ERR(sh,
> -				    "Error while waiting to get direct %s at %s.",
> -				    lock_name, lock_file);
> -				close(fd);
> -				return -1;
> -			}
> -			origtime.tv_sec--;
> -		}
> -	} while (origtime.tv_sec > 0 || sh->timeout == -1);
> -	if (!got_lock) {
> -		ERR(sh, "Could not get direct %s at %s.", lock_name, lock_file);
> +	if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) {
> +		ERR(sh, "Could not set close-on-exec for %s at %s.", lock_name,
> +		    lock_file);
>  		close(fd);
> +		free(pid_string);
> +		free(lock_pid_string);
> +		unlink(lock_file);
>  		return -1;
>  	}
> +
> +	free(pid_string);
> +	free(lock_pid_string);
>  	return fd;
>  }
>  
> @@ -1901,29 +2000,27 @@ int semanage_get_active_lock(semanage_ha
>  	}
>  }
>  
> -/* Releases the transaction lock.  Does nothing if there was not one already
> - * there. */
> +/* Releases the transaction lock: the lock file is removed */
>  void semanage_release_trans_lock(semanage_handle_t * sh)
>  {
>  	int errsv = errno;
>  	if (sh->u.direct.translock_file_fd >= 0) {
> -		flock(sh->u.direct.translock_file_fd, LOCK_UN);
>  		close(sh->u.direct.translock_file_fd);
>  		sh->u.direct.translock_file_fd = -1;
>  	}
> +	unlink(semanage_files[SEMANAGE_TRANS_LOCK]);
>  	errno = errsv;
>  }
>  
> -/* Releases the read lock.  Does nothing if there was not one already
> - * there. */
> +/* Releases the read lock: the lock file is removed */
>  void semanage_release_active_lock(semanage_handle_t * sh)
>  {
>  	int errsv = errno;
>  	if (sh->u.direct.activelock_file_fd >= 0) {
> -		flock(sh->u.direct.activelock_file_fd, LOCK_UN);
>  		close(sh->u.direct.activelock_file_fd);
>  		sh->u.direct.activelock_file_fd = -1;
>  	}
> +	unlink(semanage_files[SEMANAGE_READ_LOCK]);
>  	errno = errsv;
>  }
>  

Re: [PATCH v3] libsemanage: remove lock files

[Stephen Smalley]

On Wed, 2017-04-26 at 20:03 +0800, Jason Zaman wrote:
> On Tue, Apr 25, 2017 at 10:35:17PM +0200, Guido Trentalancia wrote:
> > Do not use flock() for file locking, but instead use generic text
> > files
> > that keep track of the process ID (PID) of the locking process.
> > 
> > Remove semanage read and transaction lock files upon releasing
> > them.
> > 
> > This third version fixes a bug in the previous version and also
> > applies
> > cleanly to the latest git tree.
> > 
> > Signed-off-by: Guido Trentalancia <guido@trentalancia.net>
> > ---
> >  src/Makefile         |    2
> >  src/semanage_store.c |  214 +++++++++++++++++++++++++++++++++++++-
> > -------------
> >  2 files changed, 160 insertions(+), 56 deletions(-)
> > 
> > --- a/src/Makefile	2017-04-25 22:27:38.105555427 +0200
> > +++ b/src/Makefile	2017-04-25 22:28:58.512555098 +0200
> > @@ -91,7 +91,7 @@ $(LIBA): $(OBJS)
> >  	$(RANLIB) $@
> >  
> >  $(LIBSO): $(LOBJS)
> > -	$(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lsepol
> > -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-
> > script=libsemanage.map,-z,defs
> > +	$(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lm -lsepol
> > -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-
> > script=libsemanage.map,-z,defs
> >  	ln -sf $@ $(TARGET)
> >  
> >  $(LIBPC): $(LIBPC).in ../VERSION
> > --- a/src/semanage_store.c	2017-04-20 16:30:21.218209972
> > +0200
> > +++ b/src/semanage_store.c	2017-04-25 22:24:35.883556172
> > +0200
> > @@ -45,6 +45,8 @@ typedef struct dbase_policydb dbase_t;
> >  #include <dirent.h>
> >  #include <errno.h>
> >  #include <fcntl.h>
> > +#include <math.h>
> > +#include <signal.h>
> >  #include <stdio.h>
> >  #include <stdio_ext.h>
> >  #include <stdlib.h>
> > @@ -52,11 +54,24 @@ typedef struct dbase_policydb dbase_t;
> >  #include <unistd.h>
> >  #include <sys/file.h>
> >  #include <sys/stat.h>
> > +#include <sys/sysctl.h>
> >  #include <sys/types.h>
> >  #include <sys/wait.h>
> >  #include <limits.h>
> >  #include <libgen.h>
> >  
> > +#include <linux/sysctl.h>
> > +
> > +#ifndef CONFIG_BASE_SMALL
> > +#define CONFIG_BASE_SMALL       0
> > +#endif
> > +
> > +#include <linux/threads.h>
> > +
> > +#ifndef PID_MAX_DEFAULT
> > +#define PID_MAX_DEFAULT 32768
> > +#endif
> > +
> >  #include "debug.h"
> >  #include "utilities.h"
> >  
> > @@ -76,6 +91,8 @@ enum semanage_file_defs {
> >  static char
> > *semanage_paths[SEMANAGE_NUM_STORES][SEMANAGE_STORE_NUM_PATHS];
> >  static char *semanage_files[SEMANAGE_NUM_FILES] = { NULL };
> >  static int semanage_paths_initialized = 0;
> > +static int pid_max;
> > +static ssize_t pid_max_length;
> >  
> >  /* These are paths relative to the bottom of the module store */
> >  static const char *semanage_relative_files[SEMANAGE_NUM_FILES] = {
> > @@ -427,8 +442,23 @@ cleanup:
> >  int semanage_check_init(semanage_handle_t *sh, const char *prefix)
> >  {
> >  	int rc;
> > +	int fd;
> > +	char root[PATH_MAX];
> > +	ssize_t amount_read;
> > +
> >  	if (semanage_paths_initialized == 0) {
> > -		char root[PATH_MAX];
> > +		pid_max = PID_MAX_DEFAULT;
> > +		pid_max_length = ceil(log10(PID_MAX_DEFAULT + 1));
> > +
> > +		fd = open("/proc/sys/kernel/pid_max", O_RDONLY);
> > +		if (fd > 0) {
> > +			char sysctlstring[pid_max_length];
> > +			amount_read = read(fd, sysctlstring,
> > pid_max_length);
> > +			if (amount_read > 0) {
> > +				pid_max = atoi(sysctlstring);
> > +				pid_max_length =
> > ceil(log10(pid_max + 1));
> > +			}
> > +		}
> >  
> >  		rc = snprintf(root,
> >  			      sizeof(root),
> > @@ -528,16 +558,23 @@ char *semanage_conf_path(void)
> >  
> >  /**************** functions that create module store
> > ***************/
> >  
> > -/* Check that the semanage store exists.  If 'create' is non-zero
> > then
> > - * create the directories.  Returns 0 if module store exists
> > (either
> > - * already or just created), -1 if does not exist or could not be
> > - * read, or -2 if it could not create the store. */
> > +/* Check that the semanage store exists and that the read lock can
> > be
> > + * taken.  If 'create' is non-zero then it creates the directories
> > + * and the lock file.  Returns 0 if the module store exists
> > (either
> > + * already or just created) and the read lock can be taken, -1 if
> > it
> > + * does not exist or it is not possible to read from it, or -2 if
> > it
> > + * could not create the store or it could not take the lock file.
> > */
> >  int semanage_create_store(semanage_handle_t * sh, int create)
> >  {
> >  	struct stat sb;
> >  	int mode_mask = R_OK | W_OK | X_OK;
> >  	const char *path = semanage_files[SEMANAGE_ROOT];
> >  	int fd;
> > +	pid_t pid, lock_pid;
> > +	char *pid_string, *lock_pid_string;
> > +	size_t pid_length;
> > +	ssize_t pid_bytes;
> > +	int invalid_lock = 0;
> >  
> >  	if (stat(path, &sb) == -1) {
> >  		if (errno == ENOENT && create) {
> > @@ -607,24 +644,81 @@ int semanage_create_store(semanage_handl
> >  			return -1;
> >  		}
> >  	}
> > +	pid = getpid();
> > +	pid_string = malloc(pid_max_length * sizeof(char));
> > +	sprintf(pid_string, "%d", pid);
> > +	pid_length = strlen(pid_string);
> >  	path = semanage_files[SEMANAGE_READ_LOCK];
> >  	if (stat(path, &sb) == -1) {
> >  		if (errno == ENOENT && create) {
> >  			if ((fd = creat(path, S_IRUSR | S_IWUSR))
> > == -1) {
> >  				ERR(sh, "Could not create lock
> > file at %s.",
> >  				    path);
> > +				free(pid_string);
> >  				return -2;
> >  			}
> > +			pid_bytes = write(fd, pid_string,
> > pid_length);
> 
> I'm pretty sure there is a race here. and another one between stat()
> and
> creat(). you can have two processes create and truncate the file then
> one write on top of the other. leaving lock files around is
> definitely
> the safest option. and are 0 bytes so its not like you're wasting any
> disk space.
> 
> Overall I disagree with changing this just because a random file is
> left
> around. NFS may be a legit reason to change but as russel said, I
> think
> locks work somewhat over NFS too. Also, NFS mounting /home or /usr is
> common but /var is pretty commonly not NFS mounted.

I agree; absent a demonstration of an actual problem with the current
libsemanage locking scheme, let's leave it alone.  Keeping around two
empty files is not a problem and this is a fairly well-established
paradigm for locking on Linux.  flock(2) works over NFS since Linux
2.6.12.

> 
> -- Jason
> 
> >  			close(fd);
> > +			free(pid_string);
> > +			if (pid_bytes == (ssize_t) pid_length)
> > +				return 0;
> > +			else {
> > +				ERR(sh, "Could not create lock
> > file at %s.", path);
> > +				return -2;
> > +			}
> >  		} else {
> >  			ERR(sh, "Could not read lock file at %s.",
> > path);
> > +			free(pid_string);
> >  			return -1;
> >  		}
> >  	} else {
> >  		if (!S_ISREG(sb.st_mode) || access(path, R_OK |
> > W_OK) == -1) {
> >  			ERR(sh, "Could not access lock file at
> > %s.", path);
> > +			free(pid_string);
> >  			return -1;
> > -		}
> > +		} else {
> > +			fd = open(path, O_RDWR);
> > +			if (fd > 0) {
> > +				lock_pid_string =
> > malloc(pid_max_length * sizeof(char));
> > +				pid_bytes = read(fd,
> > lock_pid_string, pid_max_length);
> > +				if (pid_bytes > 0) {
> > +					lock_pid = (pid_t)
> > atoi(lock_pid_string);
> > +					if (lock_pid > pid_max)
> > +						invalid_lock = 1;
> > +					else if (kill(lock_pid, 0)
> > != 0)
> > +						if (errno ==
> > ESRCH)
> > +							invalid_lo
> > ck = 1;
> > +
> > +					if (!invalid_lock) {
> > +						ERR(sh, "Could not
> > create lock file at %s.", path);
> > +						close(fd);
> > +						free(pid_string);
> > +						free(lock_pid_stri
> > ng);
> > +						return -2;
> > +					}
> > +				} else
> > +					invalid_lock = 1;
> > +
> > +				if (invalid_lock) {
> > +					pid_bytes = pwrite(fd,
> > pid_string, pid_length, 0);
> > +					close(fd);
> > +					free(pid_string);
> > +					free(lock_pid_string);
> > +					if (pid_bytes == (ssize_t)
> > pid_length)
> > +						return 0;
> > +					else {
> > +						ERR(sh, "Could not
> > create lock file at %s.", path);
> > +						return -2;
> > +					}
> > +				} else {
> > +					ERR(sh, "Could not create
> > lock file at %s.", path);
> > +					close(fd);
> > +					free(pid_string);
> > +					free(lock_pid_string);
> > +					return -2;
> > +				}
> > +			}
> > +		}	
> >  	}
> >  	return 0;
> >  }
> > @@ -1795,64 +1884,74 @@ int semanage_install_sandbox(semanage_ha
> >  static int semanage_get_lock(semanage_handle_t * sh,
> >  			     const char *lock_name, const char
> > *lock_file)
> >  {
> > +	pid_t pid, lock_pid;
> > +	char *pid_string, *lock_pid_string;
> >  	int fd;
> > -	struct timeval origtime, curtime;
> > +	size_t pid_length;
> > +	ssize_t pid_bytes;
> >  	int got_lock = 0;
> > +	int overwritten_lock = 0;
> >  
> > -	if ((fd = open(lock_file, O_RDONLY)) == -1) {
> > -		if ((fd =
> > -		     open(lock_file, O_RDWR | O_CREAT | O_TRUNC,
> > -			  S_IRUSR | S_IWUSR)) == -1) {
> > +	pid = getpid();
> > +	pid_string = malloc(pid_max_length * sizeof(char));
> > +	sprintf(pid_string, "%d", pid);
> > +	pid_length = strlen(pid_string);
> > +	if ((fd = open(lock_file, O_RDWR | O_CREAT, S_IRUSR |
> > S_IWUSR)) == -1) {
> >  			ERR(sh, "Could not open direct %s at %s.",
> > lock_name,
> >  			    lock_file);
> > +			free(pid_string);
> >  			return -1;
> > +	} else {
> > +		lock_pid_string = malloc(pid_max_length *
> > sizeof(char));
> > +		pid_bytes = pread(fd, lock_pid_string,
> > pid_max_length, 0);
> > +		if (pid_bytes == 0) {
> > +			overwritten_lock = 1;
> > +			pid_bytes = write(fd, pid_string,
> > pid_length);
> > +			if (pid_bytes == (ssize_t) pid_length)
> > +				got_lock = 1;
> > +		} else {
> > +			lock_pid = (pid_t) atoi(lock_pid_string);
> > +			if (lock_pid > pid_max) {
> > +				overwritten_lock = 1;
> > +				pid_bytes = pwrite(fd, pid_string,
> > pid_length, 0);
> > +				if (pid_bytes == (ssize_t)
> > pid_length)
> > +					got_lock = 1;
> > +			} else {
> > +				if (lock_pid == pid)
> > +				       got_lock = 1;
> > +				else if (kill(lock_pid, 0) != 0)
> > +					if (errno == ESRCH) {
> > +						overwritten_lock =
> > 1;
> > +						pid_bytes =
> > pwrite(fd, pid_string, pid_length, 0);
> > +						if (pid_bytes ==
> > (ssize_t) pid_length)
> > +							got_lock =
> > 1;
> > +					}
> > +			}
> >  		}
> >  	}
> > -	if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) {
> > -		ERR(sh, "Could not set close-on-exec for %s at
> > %s.", lock_name,
> > -		    lock_file);
> > +
> > +	if (!got_lock) {
> >  		close(fd);
> > +		free(pid_string);
> > +		free(lock_pid_string);
> > +		if (overwritten_lock)
> > +			unlink(lock_file);
> > +		ERR(sh, "Could not get direct %s at %s.",
> > lock_name, lock_file);
> >  		return -1;
> >  	}
> >  
> > -	if (sh->timeout == 0) {
> > -		/* return immediately */
> > -		origtime.tv_sec = 0;
> > -	} else {
> > -		origtime.tv_sec = sh->timeout;
> > -	}
> > -	origtime.tv_usec = 0;
> > -	do {
> > -		curtime.tv_sec = 1;
> > -		curtime.tv_usec = 0;
> > -		if (flock(fd, LOCK_EX | LOCK_NB) == 0) {
> > -			got_lock = 1;
> > -			break;
> > -		} else if (errno != EAGAIN) {
> > -			ERR(sh, "Error obtaining direct %s at
> > %s.", lock_name,
> > -			    lock_file);
> > -			close(fd);
> > -			return -1;
> > -		}
> > -		if (origtime.tv_sec > 0 || sh->timeout == -1) {
> > -			if (select(0, NULL, NULL, NULL, &curtime)
> > == -1) {
> > -				if (errno == EINTR) {
> > -					continue;
> > -				}
> > -				ERR(sh,
> > -				    "Error while waiting to get
> > direct %s at %s.",
> > -				    lock_name, lock_file);
> > -				close(fd);
> > -				return -1;
> > -			}
> > -			origtime.tv_sec--;
> > -		}
> > -	} while (origtime.tv_sec > 0 || sh->timeout == -1);
> > -	if (!got_lock) {
> > -		ERR(sh, "Could not get direct %s at %s.",
> > lock_name, lock_file);
> > +	if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) {
> > +		ERR(sh, "Could not set close-on-exec for %s at
> > %s.", lock_name,
> > +		    lock_file);
> >  		close(fd);
> > +		free(pid_string);
> > +		free(lock_pid_string);
> > +		unlink(lock_file);
> >  		return -1;
> >  	}
> > +
> > +	free(pid_string);
> > +	free(lock_pid_string);
> >  	return fd;
> >  }
> >  
> > @@ -1901,29 +2000,27 @@ int semanage_get_active_lock(semanage_ha
> >  	}
> >  }
> >  
> > -/* Releases the transaction lock.  Does nothing if there was not
> > one already
> > - * there. */
> > +/* Releases the transaction lock: the lock file is removed */
> >  void semanage_release_trans_lock(semanage_handle_t * sh)
> >  {
> >  	int errsv = errno;
> >  	if (sh->u.direct.translock_file_fd >= 0) {
> > -		flock(sh->u.direct.translock_file_fd, LOCK_UN);
> >  		close(sh->u.direct.translock_file_fd);
> >  		sh->u.direct.translock_file_fd = -1;
> >  	}
> > +	unlink(semanage_files[SEMANAGE_TRANS_LOCK]);
> >  	errno = errsv;
> >  }
> >  
> > -/* Releases the read lock.  Does nothing if there was not one
> > already
> > - * there. */
> > +/* Releases the read lock: the lock file is removed */
> >  void semanage_release_active_lock(semanage_handle_t * sh)
> >  {
> >  	int errsv = errno;
> >  	if (sh->u.direct.activelock_file_fd >= 0) {
> > -		flock(sh->u.direct.activelock_file_fd, LOCK_UN);
> >  		close(sh->u.direct.activelock_file_fd);
> >  		sh->u.direct.activelock_file_fd = -1;
> >  	}
> > +	unlink(semanage_files[SEMANAGE_READ_LOCK]);
> >  	errno = errsv;
> >  }
> >  

Re: [PATCH v3] libsemanage: remove lock files

[Guido Trentalancia]

Hello.

On Wed, 26/04/2017 at 08.56 -0400, Stephen Smalley wrote:
> On Wed, 2017-04-26 at 20:03 +0800, Jason Zaman wrote:
> > On Tue, Apr 25, 2017 at 10:35:17PM +0200, Guido Trentalancia wrote:
> > > Do not use flock() for file locking, but instead use generic text
> > > files
> > > that keep track of the process ID (PID) of the locking process.
> > > 
> > > Remove semanage read and transaction lock files upon releasing
> > > them.
> > > 
> > > This third version fixes a bug in the previous version and also
> > > applies
> > > cleanly to the latest git tree.
> > > 
> > > Signed-off-by: Guido Trentalancia <guido@trentalancia.net>
> > > ---
> > >  src/Makefile         |    2
> > >  src/semanage_store.c |  214
> > > +++++++++++++++++++++++++++++++++++++-
> > > -------------
> > >  2 files changed, 160 insertions(+), 56 deletions(-)
> > > 
> > > --- a/src/Makefile	2017-04-25 22:27:38.105555427 +0200
> > > +++ b/src/Makefile	2017-04-25 22:28:58.512555098 +0200
> > > @@ -91,7 +91,7 @@ $(LIBA): $(OBJS)
> > >  	$(RANLIB) $@
> > >  
> > >  $(LIBSO): $(LOBJS)
> > > -	$(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lsepol
> > > -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-
> > > script=libsemanage.map,-z,defs
> > > +	$(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lm -lsepol
> > > -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-
> > > script=libsemanage.map,-z,defs
> > >  	ln -sf $@ $(TARGET)
> > >  
> > >  $(LIBPC): $(LIBPC).in ../VERSION
> > > --- a/src/semanage_store.c	2017-04-20 16:30:21.218209972
> > > +0200
> > > +++ b/src/semanage_store.c	2017-04-25 22:24:35.883556172
> > > +0200
> > > @@ -45,6 +45,8 @@ typedef struct dbase_policydb dbase_t;
> > >  #include <dirent.h>
> > >  #include <errno.h>
> > >  #include <fcntl.h>
> > > +#include <math.h>
> > > +#include <signal.h>
> > >  #include <stdio.h>
> > >  #include <stdio_ext.h>
> > >  #include <stdlib.h>
> > > @@ -52,11 +54,24 @@ typedef struct dbase_policydb dbase_t;
> > >  #include <unistd.h>
> > >  #include <sys/file.h>
> > >  #include <sys/stat.h>
> > > +#include <sys/sysctl.h>
> > >  #include <sys/types.h>
> > >  #include <sys/wait.h>
> > >  #include <limits.h>
> > >  #include <libgen.h>
> > >  
> > > +#include <linux/sysctl.h>
> > > +
> > > +#ifndef CONFIG_BASE_SMALL
> > > +#define CONFIG_BASE_SMALL       0
> > > +#endif
> > > +
> > > +#include <linux/threads.h>
> > > +
> > > +#ifndef PID_MAX_DEFAULT
> > > +#define PID_MAX_DEFAULT 32768
> > > +#endif
> > > +
> > >  #include "debug.h"
> > >  #include "utilities.h"
> > >  
> > > @@ -76,6 +91,8 @@ enum semanage_file_defs {
> > >  static char
> > > *semanage_paths[SEMANAGE_NUM_STORES][SEMANAGE_STORE_NUM_PATHS];
> > >  static char *semanage_files[SEMANAGE_NUM_FILES] = { NULL };
> > >  static int semanage_paths_initialized = 0;
> > > +static int pid_max;
> > > +static ssize_t pid_max_length;
> > >  
> > >  /* These are paths relative to the bottom of the module store */
> > >  static const char *semanage_relative_files[SEMANAGE_NUM_FILES] =
> > > {
> > > @@ -427,8 +442,23 @@ cleanup:
> > >  int semanage_check_init(semanage_handle_t *sh, const char
> > > *prefix)
> > >  {
> > >  	int rc;
> > > +	int fd;
> > > +	char root[PATH_MAX];
> > > +	ssize_t amount_read;
> > > +
> > >  	if (semanage_paths_initialized == 0) {
> > > -		char root[PATH_MAX];
> > > +		pid_max = PID_MAX_DEFAULT;
> > > +		pid_max_length = ceil(log10(PID_MAX_DEFAULT +
> > > 1));
> > > +
> > > +		fd = open("/proc/sys/kernel/pid_max", O_RDONLY);
> > > +		if (fd > 0) {
> > > +			char sysctlstring[pid_max_length];
> > > +			amount_read = read(fd, sysctlstring,
> > > pid_max_length);
> > > +			if (amount_read > 0) {
> > > +				pid_max = atoi(sysctlstring);
> > > +				pid_max_length =
> > > ceil(log10(pid_max + 1));
> > > +			}
> > > +		}
> > >  
> > >  		rc = snprintf(root,
> > >  			      sizeof(root),
> > > @@ -528,16 +558,23 @@ char *semanage_conf_path(void)
> > >  
> > >  /**************** functions that create module store
> > > ***************/
> > >  
> > > -/* Check that the semanage store exists.  If 'create' is non-
> > > zero
> > > then
> > > - * create the directories.  Returns 0 if module store exists
> > > (either
> > > - * already or just created), -1 if does not exist or could not
> > > be
> > > - * read, or -2 if it could not create the store. */
> > > +/* Check that the semanage store exists and that the read lock
> > > can
> > > be
> > > + * taken.  If 'create' is non-zero then it creates the
> > > directories
> > > + * and the lock file.  Returns 0 if the module store exists
> > > (either
> > > + * already or just created) and the read lock can be taken, -1
> > > if
> > > it
> > > + * does not exist or it is not possible to read from it, or -2
> > > if
> > > it
> > > + * could not create the store or it could not take the lock
> > > file.
> > > */
> > >  int semanage_create_store(semanage_handle_t * sh, int create)
> > >  {
> > >  	struct stat sb;
> > >  	int mode_mask = R_OK | W_OK | X_OK;
> > >  	const char *path = semanage_files[SEMANAGE_ROOT];
> > >  	int fd;
> > > +	pid_t pid, lock_pid;
> > > +	char *pid_string, *lock_pid_string;
> > > +	size_t pid_length;
> > > +	ssize_t pid_bytes;
> > > +	int invalid_lock = 0;
> > >  
> > >  	if (stat(path, &sb) == -1) {
> > >  		if (errno == ENOENT && create) {
> > > @@ -607,24 +644,81 @@ int semanage_create_store(semanage_handl
> > >  			return -1;
> > >  		}
> > >  	}
> > > +	pid = getpid();
> > > +	pid_string = malloc(pid_max_length * sizeof(char));
> > > +	sprintf(pid_string, "%d", pid);
> > > +	pid_length = strlen(pid_string);
> > >  	path = semanage_files[SEMANAGE_READ_LOCK];
> > >  	if (stat(path, &sb) == -1) {
> > >  		if (errno == ENOENT && create) {
> > >  			if ((fd = creat(path, S_IRUSR |
> > > S_IWUSR))
> > > == -1) {
> > >  				ERR(sh, "Could not create lock
> > > file at %s.",
> > >  				    path);
> > > +				free(pid_string);
> > >  				return -2;
> > >  			}
> > > +			pid_bytes = write(fd, pid_string,
> > > pid_length);
> > 
> > I'm pretty sure there is a race here. and another one between
> > stat()
> > and
> > creat(). you can have two processes create and truncate the file
> > then
> > one write on top of the other. leaving lock files around is
> > definitely
> > the safest option. and are 0 bytes so its not like you're wasting
> > any
> > disk space.
> > 
> > Overall I disagree with changing this just because a random file is
> > left
> > around. NFS may be a legit reason to change but as russel said, I
> > think
> > locks work somewhat over NFS too. Also, NFS mounting /home or /usr
> > is
> > common but /var is pretty commonly not NFS mounted.
> 
> I agree; absent a demonstration of an actual problem with the current
> libsemanage locking scheme, let's leave it alone.  Keeping around two
> empty files is not a problem and this is a fairly well-established
> paradigm for locking on Linux.  flock(2) works over NFS since Linux
> 2.6.12.

If I am not wrong, I found the information on the following web page:

https://linux.die.net/man/2/flock

It looks like a manual page, but I don't know if that information is
obsolete or not (there's no date, that's the bad side of some of the
web).

There are other reports online, I have not tested so I cannot tell.

As for the problem pointed out by Jason, I have improved it by removing
the code that leads to the other race condition: if you need a revised
version (v4) of the patch, please let me know, otherwise I take there
isn't enough interest in the change or the information is
wrong/outdated.

Regards,

Guido

Re: [PATCH v3] libsemanage: remove lock files

[Stephen Smalley]

On Wed, 2017-04-26 at 20:13 +0200, Guido Trentalancia wrote:
> Hello.
> 
> On Wed, 26/04/2017 at 08.56 -0400, Stephen Smalley wrote:
> > On Wed, 2017-04-26 at 20:03 +0800, Jason Zaman wrote:
> > > On Tue, Apr 25, 2017 at 10:35:17PM +0200, Guido Trentalancia
> > > wrote:
> > > > Do not use flock() for file locking, but instead use generic
> > > > text
> > > > files
> > > > that keep track of the process ID (PID) of the locking process.
> > > > 
> > > > Remove semanage read and transaction lock files upon releasing
> > > > them.
> > > > 
> > > > This third version fixes a bug in the previous version and also
> > > > applies
> > > > cleanly to the latest git tree.
> > > > 
> > > > Signed-off-by: Guido Trentalancia <guido@trentalancia.net>
> > > > ---
> > > >  src/Makefile         |    2
> > > >  src/semanage_store.c |  214
> > > > +++++++++++++++++++++++++++++++++++++-
> > > > -------------
> > > >  2 files changed, 160 insertions(+), 56 deletions(-)
> > > > 
> > > > --- a/src/Makefile	2017-04-25 22:27:38.105555427 +0200
> > > > +++ b/src/Makefile	2017-04-25 22:28:58.512555098 +0200
> > > > @@ -91,7 +91,7 @@ $(LIBA): $(OBJS)
> > > >  	$(RANLIB) $@
> > > >  
> > > >  $(LIBSO): $(LOBJS)
> > > > -	$(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lsepol
> > > > -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-
> > > > script=libsemanage.map,-z,defs
> > > > +	$(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lm
> > > > -lsepol
> > > > -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-
> > > > script=libsemanage.map,-z,defs
> > > >  	ln -sf $@ $(TARGET)
> > > >  
> > > >  $(LIBPC): $(LIBPC).in ../VERSION
> > > > --- a/src/semanage_store.c	2017-04-20 16:30:21.218209972
> > > > +0200
> > > > +++ b/src/semanage_store.c	2017-04-25 22:24:35.883556172
> > > > +0200
> > > > @@ -45,6 +45,8 @@ typedef struct dbase_policydb dbase_t;
> > > >  #include <dirent.h>
> > > >  #include <errno.h>
> > > >  #include <fcntl.h>
> > > > +#include <math.h>
> > > > +#include <signal.h>
> > > >  #include <stdio.h>
> > > >  #include <stdio_ext.h>
> > > >  #include <stdlib.h>
> > > > @@ -52,11 +54,24 @@ typedef struct dbase_policydb dbase_t;
> > > >  #include <unistd.h>
> > > >  #include <sys/file.h>
> > > >  #include <sys/stat.h>
> > > > +#include <sys/sysctl.h>
> > > >  #include <sys/types.h>
> > > >  #include <sys/wait.h>
> > > >  #include <limits.h>
> > > >  #include <libgen.h>
> > > >  
> > > > +#include <linux/sysctl.h>
> > > > +
> > > > +#ifndef CONFIG_BASE_SMALL
> > > > +#define CONFIG_BASE_SMALL       0
> > > > +#endif
> > > > +
> > > > +#include <linux/threads.h>
> > > > +
> > > > +#ifndef PID_MAX_DEFAULT
> > > > +#define PID_MAX_DEFAULT 32768
> > > > +#endif
> > > > +
> > > >  #include "debug.h"
> > > >  #include "utilities.h"
> > > >  
> > > > @@ -76,6 +91,8 @@ enum semanage_file_defs {
> > > >  static char
> > > > *semanage_paths[SEMANAGE_NUM_STORES][SEMANAGE_STORE_NUM_PATHS];
> > > >  static char *semanage_files[SEMANAGE_NUM_FILES] = { NULL };
> > > >  static int semanage_paths_initialized = 0;
> > > > +static int pid_max;
> > > > +static ssize_t pid_max_length;
> > > >  
> > > >  /* These are paths relative to the bottom of the module store
> > > > */
> > > >  static const char *semanage_relative_files[SEMANAGE_NUM_FILES]
> > > > =
> > > > {
> > > > @@ -427,8 +442,23 @@ cleanup:
> > > >  int semanage_check_init(semanage_handle_t *sh, const char
> > > > *prefix)
> > > >  {
> > > >  	int rc;
> > > > +	int fd;
> > > > +	char root[PATH_MAX];
> > > > +	ssize_t amount_read;
> > > > +
> > > >  	if (semanage_paths_initialized == 0) {
> > > > -		char root[PATH_MAX];
> > > > +		pid_max = PID_MAX_DEFAULT;
> > > > +		pid_max_length = ceil(log10(PID_MAX_DEFAULT +
> > > > 1));
> > > > +
> > > > +		fd = open("/proc/sys/kernel/pid_max",
> > > > O_RDONLY);
> > > > +		if (fd > 0) {
> > > > +			char sysctlstring[pid_max_length];
> > > > +			amount_read = read(fd, sysctlstring,
> > > > pid_max_length);
> > > > +			if (amount_read > 0) {
> > > > +				pid_max = atoi(sysctlstring);
> > > > +				pid_max_length =
> > > > ceil(log10(pid_max + 1));
> > > > +			}
> > > > +		}
> > > >  
> > > >  		rc = snprintf(root,
> > > >  			      sizeof(root),
> > > > @@ -528,16 +558,23 @@ char *semanage_conf_path(void)
> > > >  
> > > >  /**************** functions that create module store
> > > > ***************/
> > > >  
> > > > -/* Check that the semanage store exists.  If 'create' is non-
> > > > zero
> > > > then
> > > > - * create the directories.  Returns 0 if module store exists
> > > > (either
> > > > - * already or just created), -1 if does not exist or could not
> > > > be
> > > > - * read, or -2 if it could not create the store. */
> > > > +/* Check that the semanage store exists and that the read lock
> > > > can
> > > > be
> > > > + * taken.  If 'create' is non-zero then it creates the
> > > > directories
> > > > + * and the lock file.  Returns 0 if the module store exists
> > > > (either
> > > > + * already or just created) and the read lock can be taken, -1
> > > > if
> > > > it
> > > > + * does not exist or it is not possible to read from it, or -2
> > > > if
> > > > it
> > > > + * could not create the store or it could not take the lock
> > > > file.
> > > > */
> > > >  int semanage_create_store(semanage_handle_t * sh, int create)
> > > >  {
> > > >  	struct stat sb;
> > > >  	int mode_mask = R_OK | W_OK | X_OK;
> > > >  	const char *path = semanage_files[SEMANAGE_ROOT];
> > > >  	int fd;
> > > > +	pid_t pid, lock_pid;
> > > > +	char *pid_string, *lock_pid_string;
> > > > +	size_t pid_length;
> > > > +	ssize_t pid_bytes;
> > > > +	int invalid_lock = 0;
> > > >  
> > > >  	if (stat(path, &sb) == -1) {
> > > >  		if (errno == ENOENT && create) {
> > > > @@ -607,24 +644,81 @@ int semanage_create_store(semanage_handl
> > > >  			return -1;
> > > >  		}
> > > >  	}
> > > > +	pid = getpid();
> > > > +	pid_string = malloc(pid_max_length * sizeof(char));
> > > > +	sprintf(pid_string, "%d", pid);
> > > > +	pid_length = strlen(pid_string);
> > > >  	path = semanage_files[SEMANAGE_READ_LOCK];
> > > >  	if (stat(path, &sb) == -1) {
> > > >  		if (errno == ENOENT && create) {
> > > >  			if ((fd = creat(path, S_IRUSR |
> > > > S_IWUSR))
> > > > == -1) {
> > > >  				ERR(sh, "Could not create lock
> > > > file at %s.",
> > > >  				    path);
> > > > +				free(pid_string);
> > > >  				return -2;
> > > >  			}
> > > > +			pid_bytes = write(fd, pid_string,
> > > > pid_length);
> > > 
> > > I'm pretty sure there is a race here. and another one between
> > > stat()
> > > and
> > > creat(). you can have two processes create and truncate the file
> > > then
> > > one write on top of the other. leaving lock files around is
> > > definitely
> > > the safest option. and are 0 bytes so its not like you're wasting
> > > any
> > > disk space.
> > > 
> > > Overall I disagree with changing this just because a random file
> > > is
> > > left
> > > around. NFS may be a legit reason to change but as russel said, I
> > > think
> > > locks work somewhat over NFS too. Also, NFS mounting /home or
> > > /usr
> > > is
> > > common but /var is pretty commonly not NFS mounted.
> > 
> > I agree; absent a demonstration of an actual problem with the
> > current
> > libsemanage locking scheme, let's leave it alone.  Keeping around
> > two
> > empty files is not a problem and this is a fairly well-established
> > paradigm for locking on Linux.  flock(2) works over NFS since Linux
> > 2.6.12.
> 
> If I am not wrong, I found the information on the following web page:
> 
> https://linux.die.net/man/2/flock
> 
> It looks like a manual page, but I don't know if that information is
> obsolete or not (there's no date, that's the bad side of some of the
> web).

It is out of date.  See:
http://man7.org/linux/man-pages/man2/flock.2.html

> There are other reports online, I have not tested so I cannot tell.
> 
> As for the problem pointed out by Jason, I have improved it by
> removing
> the code that leads to the other race condition: if you need a
> revised
> version (v4) of the patch, please let me know, otherwise I take there
> isn't enough interest in the change or the information is
> wrong/outdated.

Absent evidence of a problem, not interested.

Re: [PATCH v2] libsemanage: remove lock files

[Russell Coker]

On Wed, 26 Apr 2017 06:06:13 AM Guido Trentalancia wrote:
> > Pidfile locking also never works across network filesystems as pids
> > are local to 
> > a system.  You could have some combination of pid and hostname (as
> > done by 
> > some web browsers) but that gets ugly.
> 
> No, I didn't mean the complicate circumstance of NFS shared amongst
> multiple systems.
> 
> I only meant the simpler (and most common) situation where the NFS is
> mounted for only one system, therefore PIDs are unique and there is no
> need to include the hostname.

flock(2) seems to indicate that locks always worked locally on NFS filesystems 
and thus would always have worked in that case.

Please do some testing and prove that the problem occurs on NFS-root systems.

> > Really pidfiles are so horrible that one of the noteworthy features
> > of systemd 
> > is removing the need for them.
> 
> I am not that pessimistic about locking with aid of PIDs.
> 
> To be honest, the current situation has more drawbacks in my opinion.
> 
> In particular, I don't like the fact that it leaves unused lock files
> around the filesystem.

Everything else that uses lock files does that.

> > Having multiple systems operate with NFS root and a shared
> > /etc/selinux is 
> > never going to work well.  Even if everything works well (and it
> > probably 
> > won't) you will end up with systems that have the policy in
> > /etc/selinux not 
> > matching what is running.
> 
> Please forget sharing NFS. I meant dedicated NFS.
> 
> Anyway, I have created a new version of the patch that should probably
> improve the previous race condition.
> 
> If you have a way of testing a dedicated store over NFS, then I'd like
> to hear back from you the result of such testing !
> 
> But, if you are not interested in testing it, then never mind...

I think that when someone wants to change behavior that is the same as used in 
many programs they should demonstrate that it has a problem.

-- 
My Main Blog         http://etbe.coker.com.au/
My Documents Blog    http://doc.coker.com.au/