small feature

[Maxim Dounin]

Hello!

On Wed, Oct 07, 2015 at 05:53:45AM +0100, David CARLIER wrote:

> Hi all,
> 
> I developed a small feature which puts nginx in chroot mode, off by default
> and when you set a prefix path via -p, although OpenBSD team did already a
> version far before me.

I believe I've already commented mostly the same patch in trac 
ticket #805 (https://trac.nginx.org/nginx/ticket/805):

: Chrooting to the prefix looks like a wrong idea, in particular, 
: because many paths, including compiled in ones, can be outside of 
: the prefix. I would rather think of chroot()'ing to some arbitrary 
: path. The part trying to do adjustments in ngx_http_core_root() 
: looks very wrong, too, you shouldn't try to adjust anything agains 
: chroot() path at that level.

The main problem with chroot() is how to address path changes 
between master and worker processes.  And it needs some solution 
transparent to various modules.  Otherwise, it would be a real 
pain to support chroot() here and there.

I don't really see the problem addressed in the patch.  If you 
think it is, please provide details on how it's expected to work.

Some more comments below.  They are probably useless per se, as I 
think the code should be seriously rewritten to address the above 
comments, but may help you if you are planning to work further on 
nginx code.

[...]

> @@ -1014,7 +1026,6 @@

Please use

[diff]
showfunc=1

in the Mercurial configuration, it greatly simplifies reading 
patches.

>  
>  #endif
>  
> -
>      if (ccf->pid.len == 0) {
>          ngx_str_set(&ccf->pid, NGX_PID_PATH);
>      }

Please avoid unrelated (and incorrect) style changes.

> @@ -1063,6 +1074,19 @@
>      }
>  
>  
> +    if (ccf->chroot) {
> +        if (ngx_prefix) {
> +            ngx_uint_t ngx_prefixlen = ngx_strlen(ngx_prefix);

Style: variables should be defined at function start, not in the 
body; variables initialization shouldn't be done in declaration.

> +            ccf->chroot_directory.len = ngx_prefixlen;
> +            ccf->chroot_directory.data = ngx_palloc(cycle->pool, ngx_prefixlen + 1);
> +            ngx_memcpy(ccf->chroot_directory.data, ngx_prefix, ngx_prefixlen);
> +        } else if(ccf->chroot_directory.len == NGX_CONF_UNSET_UINT) {

Style: there should be a space between "if" and "(".

> +            ngx_log_error(NGX_LOG_EMERG, cycle->log, ngx_errno,
> +                    "in chroot mode the prefix path must be set ");

Style: incorrect indentation; extra space at the end of the 
message.  Further style comments suppressed, there are lots of 
style issues.

It's not clear why do you require -p for chroot.  The -p switch is 
just one of many ways to set prefix, it shouldn't be something 
specific.

It's not clear why do you test ccf->chroot_directory.len, as it's 
only set in the "if (ngx_prefix)" part above.  That is, the test 
is useless.

[...]

> +    if (cycle->prefix.data != NULL) {
> +        u_char *prefix = cycle->prefix.data;
> +        prefix[cycle->prefix.len] = '\0';

Adding NUL character here either useless (if prefix is already 
NUL-terminated), or incorrect (what guarantees that there is a 
space for NUL?).

[...]

-- 
Maxim Dounin
http://nginx.org/