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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
