| Message ID | 1444464456-27941-1-git-send-email-azhou@nicira.com |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, Oct 10, 2015 at 1:07 AM, Andy Zhou <azhou@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 <azhou@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
On Wed, Nov 4, 2015 at 6:16 PM, Ansis Atteka <ansisatteka@gmail.com> wrote: > > > On Sat, Oct 10, 2015 at 1:07 AM, Andy Zhou <azhou@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 <azhou@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 > > There are a few examples, for example: "all_bfds__" in lib/bfd.c. >> 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. This patch only adds comments. Garbage input would have been blocked by command line paring. This assert would catch the cases where a --user option is specified, but some how user switch is not done. To aid debugging, I think it is a good idea to add a log here. I will make the change. > > > > > >> } >> >> @@ -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
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; 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()); } @@ -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; }
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. Signed-off-by: Andy Zhou <azhou@nicira.com> --- lib/daemon-unix.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)