[ovs-dev,1/2] lib: simplify daemon_become_new_user__()
diff mbox

Message ID 1444464456-27941-1-git-send-email-azhou@nicira.com
State Superseded
Headers show

Commit Message

Andy Zhou Oct. 10, 2015, 8:07 a.m. UTC
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(-)

Comments

Ansis Atteka Nov. 5, 2015, 2:16 a.m. UTC | #1
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
Andy Zhou Nov. 9, 2015, 6:15 p.m. UTC | #2
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

Patch
diff mbox

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;
 }