On Thu, Sep 20, 2012 at 02:53:52PM +0200, Peter Krempa wrote:
> On 09/19/12 23:32, Marcelo Cerri wrote:
> >The DAC driver is missing parsing of group and user names for DAC labels
> >and currently just parses uid and gid. This patch extends it to support
> >names, so the following security label definition is now valid:
> >
> > <seclabel type='static' model='dac' relabel='yes'>
> > <label>qemu:qemu</label>
> > <imagelabel>qemu:qemu</imagelabel>
> > </seclabel>
> >---
> > src/security/security_dac.c | 49 ++++++++++++++++++++++++++++++++++++---------
> > 1 file changed, 40 insertions(+), 9 deletions(-)
> >
> >diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> >index be65d6e..7e11e31 100644
> >--- a/src/security/security_dac.c
> >+++ b/src/security/security_dac.c
> >@@ -66,28 +66,59 @@ void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr,
> > }
> >
> > static
> >+int parseId(const char *str, unsigned int *id)
> >+{
> >+ char *endptr = NULL;
> >+
> >+ if (str == NULL || id == NULL)
> >+ return -1;
> >+
> >+ if (virStrToLong_ui(str, &endptr, 10, id) || endptr != NULL)
>
> After successful parse "endptr" will point to the '\0' byte at the
> end of the string always thiggering this condition. Call
> virStrToLong_ui with NULL instead of endptr and the wrapper will do
> the expected thing.
You're right.
>
> >+ return -1;
> >+
> >+ return 0;
> >+}
> >+
> >+static
> > int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
> > {
> >+ int rc = -1;
> > unsigned int theuid;
> > unsigned int thegid;
> >- char *endptr = NULL;
> >+ char *sep = NULL;
> >+ char *tmp_label = NULL;
> >
> > if (label == NULL)
> >- return -1;
> >+ goto done;
> >
> >- if (virStrToLong_ui(label, &endptr, 10, &theuid) ||
> >- endptr == NULL || *endptr != ':') {
> >- return -1;
> >- }
> >+ tmp_label = strdup(label);
> >+ if (tmp_label == NULL)
>
> You need to raise an OOM error with virReportOOMError() here even if
> it's masked by the upper layer. This error gets recorded to the logs
> and might help debugging.
I agree.
>
> I'm thinking if it's worth reporting other errors in this function
> as they get masked. They might be useful for debugging purposes even
> if they don't get propagated to the user.
Do you think that describing each specific error with VIR_DEBUG should
be enough?
>
> >+ goto done;
> >
> >- if (virStrToLong_ui(endptr + 1, NULL, 10, &thegid))
> >- return -1;
> >+ sep = strchr(tmp_label, ':');
> >+ if (sep == NULL)
> >+ goto done;
> >+ *sep = '\0';
> >+
> >+ if (virGetUserID(tmp_label, &theuid) < 0 &&
> >+ parseId(tmp_label, &theuid) < 0)
> >+ goto done;
> >+
> >+ if (virGetGroupID(sep + 1, &thegid) < 0 &&
> >+ parseId(sep + 1, &thegid) < 0)
> >+ goto done;
> >
> > if (uidPtr)
> > *uidPtr = theuid;
> > if (gidPtr)
> > *gidPtr = thegid;
> >- return 0;
> >+
> >+ rc = 0;
> >+
> >+done:
>
> I'd rename this label to "cleanup" for consistency with other libvirt code.
Ok.
>
> >+ VIR_FREE(tmp_label);
> >+
> >+ return rc;
> > }
> >
> > /* returns 1 if label isn't found, 0 on success, -1 on error */
> >
>
> Peter
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list