[libvirt] [PATCHv2 2/2] security_dac: compute supplemental groups before fork

[Michal Privoznik]

On 18.07.2013 01:08, Eric Blake wrote:
> Commit 75c1256 states that virGetGroupList must not be called
> between fork and exec, then commit ee777e99 promptly violated
> that for lxc's use of virSecurityManagerSetProcessLabel.  Hoist
> the supplemental group detection to the time that the security
> manager is created.  Qemu is safe, as it uses
> virSecurityManagerSetChildProcessLabel which in turn uses
> virCommand to determine supplemental groups.
> 
> This does not fix the fact that virSecurityManagerSetProcessLabel
> calls virSecurityDACParseIds calls parseIds which eventually
> calls getpwnam_r, which also violates fork/exec async-signal-safe
> safety rules, but so far no one has complained of hitting
> deadlock in that case.
> 
> * src/security/security_dac.c (_virSecurityDACData): Track groups
> in private data.
> (virSecurityDACPreFork): New function, to set them.
> (virSecurityDACClose): Clean up new fields.
> (virSecurityDACGetIds): Alter signature.
> (virSecurityDACSetSecurityHostdevLabelHelper)
> (virSecurityDACSetChardevLabel, virSecurityDACSetProcessLabel)
> (virSecurityDACSetChildProcessLabel): Update callers.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/security/security_dac.c | 63 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 44 insertions(+), 19 deletions(-)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 9b5eaa8..00d04bf 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -43,6 +43,8 @@ typedef virSecurityDACData *virSecurityDACDataPtr;
>  struct _virSecurityDACData {
>      uid_t user;
>      gid_t group;
> +    gid_t *groups;
> +    int ngroups;
>      bool dynamicOwnership;
>  };
> 
> @@ -146,7 +148,8 @@ virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr)
> 
>  static int
>  virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv,
> -                     uid_t *uidPtr, gid_t *gidPtr)
> +                     uid_t *uidPtr, gid_t *gidPtr,
> +                     gid_t **groups, int *ngroups)
>  {
>      int ret;
> 
> @@ -157,8 +160,13 @@ virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv,
>          return -1;
>      }
> 
> -    if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0)
> +    if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0) {
> +        if (groups)
> +            *groups = NULL;
> +        if (ngroups)
> +            ngroups = 0;

I believe you wanted *ngroups = 0; in here.

>          return ret;
> +    }
> 
>      if (!priv) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,

Michal