On 09/19/2012 02:42 PM, Peter Krempa wrote:

The DAC security driver silently ignored errors when parsing the DAC label and used default values instead.

With a domain containing the following label definition:

<seclabel type='static' model='dac' relabel='yes'> <label>sdfklsdjlfjklsdjkl</label> </seclabel>

the domain would start normaly but the disk images would be still owned by root and no error was displayed.

This patch changes the behavior if the parsing of the label fails (note that a not present label is not a failure and in this case the default label should be used) the error isn't masked but is raised that causes the domain start to fail with a descriptive error message:

virsh # start tr error: Failed to start domain tr error: internal error invalid argument: failed to parse DAC label 'sdfklsdjlfjklsdjkl' for domain 'tr'

I also changed the error code to "invalid argument" from "internal error" and tweaked the various error messages to contain correct and useful information. --- Diff to v2: - Fixed all error reporting paths to contain useful messages - Tweaked error messages to contain more information - Fixed printing of seclabel->label instead of seclabel->imagelabel in the imagelabel parsing func.

src/security/security_dac.c | 95 +++++++++++++++++++++++++++++---------------- 1 file changed, 61 insertions(+), 34 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 211fb37..be65d6e 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -90,6 +90,7 @@ int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr) return 0; }

+/* returns 1 if label isn't found, 0 on success, -1 on error */ static int virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr) { @@ -98,20 +99,18 @@ int virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr) virSecurityLabelDefPtr seclabel;

if (def == NULL) - return -1; + return 1;

seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (seclabel == NULL || seclabel->label == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("security label for DAC not found in domain %s"), - def->name); - return -1; + VIR_DEBUG("DAC seclabel for domain '%s' wasn't found", def->name);

You use "seclabel" here, ...

+ return 1; }

if (seclabel->label && parseIds(seclabel->label, &uid, &gid)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to parse uid and gid for DAC " - "security driver: %s"), seclabel->label); + virReportError(VIR_ERR_INVALID_ARG, + _("failed to parse DAC label '%s' for domain '%s'"),

... "label" here ...

+ seclabel->label, def->name); return -1; }

@@ -127,19 +126,35 @@ static int virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv, uid_t *uidPtr, gid_t *gidPtr) { - if (virSecurityDACParseIds(def, uidPtr, gidPtr) == 0) - return 0; + int ret;

- if (priv) { - if (uidPtr) - *uidPtr = priv->user; - if (gidPtr) - *gidPtr = priv->group; - return 0; + if (!def && !priv) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to determine default DAC label "

... and here ...

+ "for an unknown object")); + return -1; } - return -1; + + if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0) + return ret; + + if (!priv) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("DAC security label couldn't be determined "

..., but "security label" here.

+ "for domain '%s'"), def->name); + return -1; + } + + if (uidPtr) + *uidPtr = priv->user; + if (gidPtr) + *gidPtr = priv->group; + + return 0; }

+ +/* returns 1 if label isn't found, 0 on success, -1 on error */ static int virSecurityDACParseImageIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr) @@ -149,21 +164,19 @@ int virSecurityDACParseImageIds(virDomainDefPtr def, virSecurityLabelDefPtr seclabel;

if (def == NULL) - return -1; + return 1;

seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (seclabel == NULL || seclabel->imagelabel == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("security label for DAC not found in domain %s"), - def->name); - return -1; + VIR_DEBUG("DAC imagelabel for domain '%s' wasn't found", def->name); + return 1; }

if (seclabel->imagelabel && parseIds(seclabel->imagelabel, &uid, &gid)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to parse uid and gid for DAC " - "security driver: %s"), seclabel->label); + virReportError(VIR_ERR_INVALID_ARG, + _("failed to parse DAC imagelabel '%s' for domain '%s'"), + seclabel->imagelabel, def->name); return -1; }

@@ -179,17 +192,31 @@ static int virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr priv, uid_t *uidPtr, gid_t *gidPtr) { - if (virSecurityDACParseImageIds(def, uidPtr, gidPtr) == 0) - return 0; + int ret;

- if (priv) { - if (uidPtr) - *uidPtr = priv->user; - if (gidPtr) - *gidPtr = priv->group; - return 0; + if (!def && !priv) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to determine default DAC imagelabel " + "for an unknown object")); + return -1; + } + + if ((ret = virSecurityDACParseImageIds(def, uidPtr, gidPtr)) <= 0) + return ret; + + if (!priv) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("DAC security imagelabel couldn't be determined "

And here you have "security imagelabel" as opposed to the other "imagelabel"-only messages.

+ "for domain '%s'"), def->name); + return -1; } - return -1; + + if (uidPtr) + *uidPtr = priv->user; + if (gidPtr) + *gidPtr = priv->group; + + return 0; }

Code seems fine, though, so ACK with unifying those messages. I suggest either "seclabel" and "imagelabel" OR 'security label" and "security imagelabel", whatever you think suits the error and XML better, but one for all errors with the same label. Martin