Re: [PATCH] BUG/MINOR: init: fix set-dumpable when using uid/gid
On Tue, Nov 19, 2019 at 10:11:36AM +0100, William Dauchy wrote: > Here is the backport for haproxy-20 tree. Now merged, thanks William. Willy
Re: [PATCH] BUG/MINOR: init: fix set-dumpable when using uid/gid
Hi, On Tue, Nov 19, 2019 at 02:42:23PM +0500, Илья Шипицин wrote: > small question. > `/proc/sys/fs/suid_dumpable` is linux specific. will it work under freebsd, > openbsd ? windows ? > also, linux might not mount that filesystem. will it work ? this code is protected around USE_PRCTL define, so I guess it is not selected on other OS. for the linux part, we do not use this path in proc fs, I was referring to that to explain the behaviour of prctl. hope it helps, -- William
Re: [PATCH] BUG/MINOR: init: fix set-dumpable when using uid/gid
вт, 19 нояб. 2019 г. в 14:15, William Dauchy :
> in mworker mode used with uid/gid settings, it was not possible to get
> a coredump despite the set-dumpable option.
> indeed prctl(2) manual page specifies the dumpable attribute is reverted
> to `/proc/sys/fs/suid_dumpable` in a few conditions such as process
> effective user and group are changed.
>
small question.
`/proc/sys/fs/suid_dumpable` is linux specific. will it work under freebsd,
openbsd ? windows ?
also, linux might not mount that filesystem. will it work ?
>
> this patch moves the whole set-dumpable logic before the polling code in
> order to catch all possible cases where we could have changed the
> uid/gid. It however does not cover the possible segfault at startup.
>
> this patch should be backported in 2.0.
>
> Signed-off-by: William Dauchy
>
> ---
>
> Hi Willy,
>
> Here is the backport for haproxy-20 tree.
>
> Thanks,
>
> William
>
> ---
> src/haproxy.c | 50 +-
> 1 file changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/src/haproxy.c b/src/haproxy.c
> index fa3fbad4..a0e630df 100644
> --- a/src/haproxy.c
> +++ b/src/haproxy.c
> @@ -2946,31 +2946,6 @@ int main(int argc, char **argv)
> }
> }
>
> - /* try our best to re-enable core dumps depending on system
> capabilities.
> -* What is addressed here :
> -* - remove file size limits
> -* - remove core size limits
> -* - mark the process dumpable again if it lost it due to
> user/group
> -*/
> - if (global.tune.options & GTUNE_SET_DUMPABLE) {
> - limit.rlim_cur = limit.rlim_max = RLIM_INFINITY;
> -
> -#if defined(RLIMIT_FSIZE)
> - if (setrlimit(RLIMIT_FSIZE, ) == -1)
> - ha_warning("[%s.main()] Failed to set the raise
> the maximum file size.\n", argv[0]);
> -#endif
> -
> -#if defined(RLIMIT_CORE)
> - if (setrlimit(RLIMIT_CORE, ) == -1)
> - ha_warning("[%s.main()] Failed to set the raise
> the core dump size.\n", argv[0]);
> -#endif
> -
> -#if defined(USE_PRCTL)
> - if (prctl(PR_SET_DUMPABLE, 1, 0, 0, 0) == -1)
> - ha_warning("[%s.main()] Failed to set the dumpable
> flag, no core will be dumped.\n", argv[0]);
> -#endif
> - }
> -
> /* check ulimits */
> limit.rlim_cur = limit.rlim_max = 0;
> getrlimit(RLIMIT_NOFILE, );
> @@ -3253,6 +3228,31 @@ int main(int argc, char **argv)
> fork_poller();
> }
>
> + /* try our best to re-enable core dumps depending on system
> capabilities.
> +* What is addressed here :
> +* - remove file size limits
> +* - remove core size limits
> +* - mark the process dumpable again if it lost it due to
> user/group
> +*/
> + if (global.tune.options & GTUNE_SET_DUMPABLE) {
> + limit.rlim_cur = limit.rlim_max = RLIM_INFINITY;
> +
> +#if defined(RLIMIT_FSIZE)
> + if (setrlimit(RLIMIT_FSIZE, ) == -1)
> + ha_warning("[%s.main()] Failed to set the raise
> the maximum file size.\n", argv[0]);
> +#endif
> +
> +#if defined(RLIMIT_CORE)
> + if (setrlimit(RLIMIT_CORE, ) == -1)
> + ha_warning("[%s.main()] Failed to set the raise
> the core dump size.\n", argv[0]);
> +#endif
> +
> +#if defined(USE_PRCTL)
> + if (prctl(PR_SET_DUMPABLE, 1, 0, 0, 0) == -1)
> + ha_warning("[%s.main()] Failed to set the dumpable
> flag, no core will be dumped.\n", argv[0]);
> +#endif
> + }
> +
> global.mode &= ~MODE_STARTING;
> /*
> * That's it : the central polling loop. Run until we stop.
> --
> 2.24.0
>
>
>
Re: [PATCH] BUG/MINOR: init: fix set-dumpable when using uid/gid
On Sun, Nov 17, 2019 at 10:54:08AM +, William Dauchy wrote: > Yes, there are different cases; I was able to reproduce it only with the > mworker mode (-W or -Ws); indeed we currently do setuid in this order: > -> global.mode & (MODE_MWORKER|MODE_DAEMON) > -> setuid/setgid > -> set dumpable > but after there is also: > -> global.mode & (MODE_DAEMON | MODE_MWORKER | MODE_MWORKER_WAIT) > -> setuid/setgid > -> this is where I added a the new set dumpable I think I didn't notice this location. > After re reading this, maybe I could simplify the code and mutualise > some code to make it clearer. Thus probably we should move it to the second place only if it's supposed to catch all of them ? Or even just before the polling loop. Oh now I think I remember why it was set early. I expected to catch some early crashes (e.g. during config processing). With some more background on this now, I'd think that we don't care much about this because this is extremely rare and easier to reproduce at will if needed. So indeed, those we're really interested in are the runtime crashes thus setting this the latest possible likely makes much more sense. Willy
Re: [PATCH] BUG/MINOR: init: fix set-dumpable when using uid/gid
Hi Willy, Thank you for the quick ansswer. On Sun, Nov 17, 2019 at 11:12:29AM +0100, Willy Tarreau wrote: > That's strange, I was absolutely certain it was done after the setuid > stuff precisely for the reason you explain above. Do you think the > sequence differs in master-worker mode maybe ? Or maybe I did something > completely wrong but given that till now it appeared to work for me, I > must admit I'm a bit puzzled. Yes, there are different cases; I was able to reproduce it only with the mworker mode (-W or -Ws); indeed we currently do setuid in this order: -> global.mode & (MODE_MWORKER|MODE_DAEMON) -> setuid/setgid -> set dumpable but after there is also: -> global.mode & (MODE_DAEMON | MODE_MWORKER | MODE_MWORKER_WAIT) -> setuid/setgid -> this is where I added a the new set dumpable After re reading this, maybe I could simplify the code and mutualise some code to make it clearer. Thanks, -- William
Re: [PATCH] BUG/MINOR: init: fix set-dumpable when using uid/gid
Hi William, On Sun, Nov 17, 2019 at 10:26:23AM +0100, William Dauchy wrote: > in mworker mode used with uid/gid settings, it was not possible to get > a coredump despite the set-dumpable option. > indeed prctl(2) manual page specifies the dumpable attribute is reverted > to `/proc/sys/fs/suid_dumpable` in a few conditions such as process > effective user and group are changed. > > this patch duplicates the prctl part done earlier in the init, but after > the uid/gid changes. There might be a cleaner way to do it and avoid > code duplication. That's strange, I was absolutely certain it was done after the setuid stuff precisely for the reason you explain above. Do you think the sequence differs in master-worker mode maybe ? Or maybe I did something completely wrong but given that till now it appeared to work for me, I must admit I'm a bit puzzled. Thanks, Willy
