On Sat, Oct 10, 2015 at 1:07 AM, Andy Zhou <az...@nicira.com> wrote:
> Global variable 'switch_user' is no longer needed to make sure
> user switch only happens once per process. Testing for uid directly
> simplifies the logic; if switch process has taken place, then the
> currnet uid can not be zero.
s/currnet/current
>
> Signed-off-by: Andy Zhou <az...@nicira.com>
> ---
> lib/daemon-unix.c | 27 +++++++++++++--------------
> 1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
> index 868e2c9..cafa397 100644
> --- a/lib/daemon-unix.c
> +++ b/lib/daemon-unix.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2015 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -83,8 +83,7 @@ static bool monitor;
>
> /* --user: Only root can use this option. Switch to new uid:gid after
> * initially running as root. */
> -static bool switch_user = false;
> -static bool non_root_user = false;
> +static bool non_root_user__ = false;
CodingStyle.md says:
- Do not use names that begin with _. If you need a name for
"internal use only", use __ as a suffix instead of a prefix.
However, I can't find precedent in OVS tree that we use this naming
convention for static variables:
git grep "__" | grep static
> static uid_t uid;
> static gid_t gid;
> static char *user = NULL;
> @@ -440,13 +439,11 @@ daemonize_start(bool access_datapath)
> assert_single_threaded();
> daemonize_fd = -1;
>
> - if (switch_user) {
> + if (non_root_user__) {
> daemon_become_new_user__(access_datapath);
> - switch_user = false;
> - }
>
> - /* If --user is specified, make sure user switch has completed by
now. */
> - if (non_root_user) {
> + /* If --user is specified, make sure this is no longer a root
> + * process. */
> ovs_assert(geteuid() && getuid());
I think this assert can be triggered also under normal conditions. With
"normal conditions" I mean the ones where user attempts to pass unexpected
(or even garbage) values through OVS user-facing interfaces (OVSDB, OF,
command line etc). Use VLOG_XXX() for such cases instead or don't use
asserts at all.
> }
>
> @@ -853,6 +850,12 @@ daemon_become_new_user_linux(bool access_datapath
OVS_UNUSED)
> static void
> daemon_become_new_user__(bool access_datapath)
> {
> + /* Execute this function at most once. After this function has been
> + * executed, current uid and effective uid can not be zero. */
> + if (getuid() || geteuid()) {
> + return;
> + }
> +
> if (LINUX) {
> if (LIBCAPNG) {
> daemon_become_new_user_linux(access_datapath);
> @@ -873,12 +876,8 @@ void
> daemon_become_new_user(bool access_datapath)
> {
> assert_single_threaded();
> - if (switch_user) {
> + if (non_root_user__) {
> daemon_become_new_user__(access_datapath);
> -
> - /* Make sure daemonize_start() will not switch
> - * user again. */
> - switch_user = false;
> }
> }
>
> @@ -1041,5 +1040,5 @@ daemon_set_new_user(const char *user_spec)
> }
> }
>
> - switch_user = non_root_user = true;
> + non_root_user__ = true;
> }
> --
> 1.9.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
