Import nginx code to set process title, use it for "cinit" #169
Conversation
|
|
|
||
| /* In upstream nginx, this is called early in main. However, | ||
| * I don't see a reason not to merge it with ngx_init_setproctitle() | ||
| */ |
There was a problem hiding this comment.
This is wrong. The parse_args_recurse() function stores references to the argument data in various places, and if the replacing of the args is done after that then those places will point to an area we're actively modifying.
There was a problem hiding this comment.
Oh, i see you call this pretty early. Should be safe then.
|
The failure is due to the saved argv/env leak, which i'm not sure we can fix. Can we suppress it? |
|
Anyway, this seems very useful to me. |
|
Oh, and do we really need to keep the ngx_ prefix? |
|
If you look at the logs, we have other problems because ASAN gets confused (understandably) by unsharing the PID namespace and forking. I'll look at keeping ASAN enabled, just with leak detection disabled. |
|
Re |
|
I prefer to give the credits in comments and have a more logical structure in our code. |
When debugging things, it's really useful to be able to see from the process output which thing is which. Here, we're using the term "cinit" for the in-container init. I looked around on the internets for this - I thought systemd would have a good implementation, but they don't do the requsite trick with malloc to ensure we have enough space. nginx does. Now, I also found https://tycho.ws/blog/2015/02/setproctitle.html but their implementation sounded way more complex. So, going from the nginx code, I heavily edited it to drop Solaris/FreeBSD etc. Also, to use `xmalloc` instead of their allocator, etc. This just makes things easier to debug; there's a risk though that it's new parsing code that is reachable in the suid case by an attacker outside; I'll do some extra review on this if we agree to take it.
cgwalters
force-pushed
the
setproctitle
branch
from
8e1b983
to
e7c53ec
Compare
|
Ok, reworked, rebased. ⬆️ |
| bwrap_os_argv = (char **) argv; | ||
| bwrap_argc = argc; | ||
|
|
||
| bwrap_argv = xmalloc((argc + 1) * sizeof(char *)); |
There was a problem hiding this comment.
I don't get this. We make this copy, but it is never used anywhere. main() still works on the original, which we will later overwrite.
|
OK, reading the nginx code, it looks like they were using the cached argv copy so they could later rewrite argv for the master process, e.g. on my system:
Where Not sure whether it's worth it; with just what we have now it's pretty obvious that the initial |
|
@cgwalters Well, if we're not going to do that, the a lot of the work duplicating the env and argv is completely unnecessary, all we need to do is to figure out exactly how large the area to overwrite is. Otoh, something like that is bound to bite us in the ass eventually when we accidentally dereference an env-var in the cinit code. |
|
☔ The latest upstream changes (presumably b6370de) made this pull request unmergeable. Please resolve the merge conflicts. |
When debugging things, it's really useful to be able to see from the process
output which thing is which. Here, we're using the term "cinit" for the
in-container init.
I looked around on the internets for this - I thought systemd would have a good
implementation, but they don't do the requsite trick with malloc to ensure we
have enough space. nginx does. Now, I also found
https://tycho.ws/blog/2015/02/setproctitle.html but their implementation
sounded way more complex.
So, going from the nginx code, I heavily edited it to drop Solaris/FreeBSD etc.
Also, to use
xmallocinstead of their allocator, etc.This just makes things easier to debug; there's a risk though that it's new
parsing code that is reachable in the suid case by an attacker outside; I'll do
some extra review on this if we agree to take it.