On Wed, Nov 4, 2015 at 6:16 PM, Ansis Atteka <ansisatt...@gmail.com> wrote:
>
>
> 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
>
>
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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
