Re: [Xen-devel] [PATCH v2 05/10] libxl: Do root checks once in libxl__domain_get_device_model_uid

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

George Dunlap writes ("[PATCH v2 05/10] libxl: Do root checks once in 
libxl__domain_get_device_model_uid"):
> At the moment, we check for equivalence to literal "root" before
> deciding whether to add the `runas` command-line option to QEMU.  This
> is unsatisfactory for several reasons.
...
> v2:
> - Refactor to use `out` rather than multiple labels
> - Only check for root once
> - Use 'out' rather than direct returns for errors (only use direct returns
>   for early `succeed-without-setting-runas` paths)
> - Use `rc` rather than `ret` to more closely align with CODING_STYLE
> - Fill out comments about the cases we're handling
> - Return ERROR_DEVICE_EXISTS rather than ERROR_FAIL if there's another
>   username that maps to our calculated uid
> - Report an error if the specified device_model_user doesn't exist

Thanks.  This is all much better now.  I still have a few style nits,
and one comment on your reuse of a libxl error code, but AFAICT the
algorithm is right.


> +        if (!user_base) {
> +            LOGD(ERROR, guest_domid, "Couldn't find device_model_user %s",
> +                 user);
> +            rc = ERROR_INVAL;
> +        } else
> +            intended_uid = user_base->pw_uid;
>  
> +        goto out;

1. Would you mind adding the missing { } ?
  | ... To avoid confusion, either all
  | the blocks in an if...else chain have braces, or none of them do.

2. Can you please set rc in the other path ?  I think this is supposed
to be a success path.  Relying on the previous value of rc seems very
action at a distance.  (Later: yes, I see this this is a success path.
See my comments about the rules implied by your `out:' block.)

TBH I would prefer two goto outs rather than one, like this:

> +        if (!user_base) {
> +            LOGD(ERROR, guest_domid, "Couldn't find device_model_user %s",
> +                 user);
> +            rc = ERROR_INVAL;
  +            goto out;
  +        } else {
> +            intended_uid = user_base->pw_uid;
  +            rc = 0;
  +            goto out;
  +        }
> +    }

which seems easier to read since it is not necessary to untangle the
whole if, and look at the context, to see the correctness of the
individual bits.

Or you could treat the !user_base as an error block and write this:

> +        if (!user_base) {
> +            LOGD(ERROR, guest_domid, "Couldn't find device_model_user %s",
> +                 user);
> +            rc = ERROR_INVAL;
  +            goto out;
  +        }
  +        intended_uid = user_base->pw_uid;
  +        rc = 0;
  +        goto out;
> +    }



> +    /*
> +     * If dm_restrict isn't set, and we don't have a specified user, don't
> +     * bother setting a `-runas` parameter.
> +     */
>      if (!libxl_defbool_val(b_info->dm_restrict)) {
>          LOGD(DEBUG, guest_domid,
>               "dm_restrict disabled, starting QEMU as root");
>          return 0;
>      }

Why `return 0' here but `goto out' earlier ?  IMO all the success
returns should be the same.

> +        rc = userlookup_helper_getpwuid(gc, intended_uid,
>                                           &user_clash_pwbuf, &user_clash);
> -        if (ret < 0)
> -            return ret;
> +        if (rc < 0)
> +            goto out;

In my last mail I suggested this should be `ret < 0' but of course now
it should be `if (rc)' ...

>          if (user_clash) {
>              LOGD(ERROR, guest_domid,
>                   "wanted to use uid %ld (%s + %d) but that is user %s !",
>                   (long)intended_uid, LIBXL_QEMU_USER_RANGE_BASE,
>                   guest_domid, user_clash->pw_name);
> -            return ERROR_FAIL;
> +            rc = ERROR_DEVICE_EXISTS;
> +            goto out;

I appreciate your desire to specify particular error value, but I am
far from convinced that ERROR_DEVICE_EXISTS is appropriate.  It would
lead someone to ask which device was in the way.

We generally use EINVAL for bad configurations.  If you prefer, feel
free to introduce a new error code.  32-bit signed integers are pretty
cheap.

> +    if (rc < 0)
> +        goto out;
>      if (user_base) {
>          LOGD(WARN, guest_domid, "Could not find user %s, falling back to %s",
>               LIBXL_QEMU_USER_RANGE_BASE, LIBXL_QEMU_USER_SHARED);
> -        goto end_search;
> +        intended_uid = user_base->pw_uid;
> +        goto out;

Here we have this pattern again with a `goto out' without a preceding
assignment to `rc'.  AFAICT the rules implied by your out block are:

 * Every goto out must be preceded by an assignment to rc.
   IMO there is no reason this should not immediately precede
   the goto out.

 * Additionally, if rc is 0 then the goto out must also be preceded
   relatively recently by an assignment to intended_uid.

> +out:
> +    if (!rc) {
> +        if (intended_uid == 0) {
> +            LOGD(ERROR, guest_domid, "intended_uid is 0 (root)!");
> +            return ERROR_INVAL;
> +        }
> +
> +        state->dm_runas = user;
> +    }
> +
> +    return rc;

TBH I dislike the early return in the `goto out' block.  If a resource
deallocation were to be introduced, that `return ERROR_INVAL' would
become a memory leak.

I think this means lifting state->dm_runas into else, or writing this:

> +out:
> +    if (!rc) {
> +        if (intended_uid == 0) {
> +            LOGD(ERROR, guest_domid, "intended_uid is 0 (root)!");
  +            rc = ERROR_INVAL;
> +        }
  +    }
  +    if (!rc) {
> +        state->dm_runas = user;
> +    }
> +
> +    return rc;

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel