apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject Re: APR-izing httpd's unixd_set_proc_mutex_perms
Date Mon, 21 Jul 2008 10:47:17 GMT
On Sat, Jul 19, 2008 at 07:13:08AM +0200, Mladen Turk wrote:
...
> Index: locks/unix/proc_mutex.c
> ===================================================================
> --- locks/unix/proc_mutex.c	(revision 677948)
> +++ locks/unix/proc_mutex.c	(working copy)
> @@ -915,6 +915,40 @@
>      return NULL;
>  }
>  
> +APR_DECLARE(apr_status_t) apr_proc_mutex_set_perms(apr_proc_mutex_t *mutex,
> +                                                   apr_fileperms_t perms,
> +                                                   apr_uid_t *uid,
> +                                                   apr_gid_t *gid)

uid/gid should be passed directly rather than by reference, I think.

> +{
> +
> +    if (!geteuid()) {

That seems out-of-place.  Either it should be an API guarantee that this 
call is a no-op if the euid is not 0, or that should be left to the 
caller.  I think the latter makes more sense.

> +#if APR_HAS_SYSVSEM_SERIALIZE
> +        if (mutex->meth == &mutex_sysv_methods) {

To be consistent with the rest of this code, the implementation should 
use add a new function in the _lock_methods_t vtable and add new 
method-specific functions.

...
> +#endif
> +#if APR_HAS_FLOCK_SERIALIZE
> +        if (mutex->meth == &mutex_flock_methods) {
> +            if (mutex->fname) {

Is this necessary?  AFAICT ->fname should always be non-NULL.

> +                if (chown(mutex->fname, *uid,
> +                          -1 /* no gid change */) < 0) {

Where available, using fchown() on the fd instead of chown() would be 
more efficient/safer.

...
>  /**
> + * Set mutex perimissions.

spello for "permissions".

> + * @param mutex the mutex to set.
> + * @param perms Access permissions for the mutex. Mimics Unix access rights.

What does "Mimics Unix access rights" mean?  How will this mimic Unix on 
non-Unix platforms?  It seems superfluous, I'd remove the phrase.

Regards,

joe

Mime
View raw message