On 09/18/2012 04:00 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: invalid argument: failed to parse uid and gid for DAC security driver: sdfklsdjlfjklsdjkl
I also changed the error code to "invalid argument" from "internal error". --- Marcelo Cerri pointed out that there's another function doing the same that I missed in v1. This version fixes both of them.
src/security/security_dac.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 211fb37..ea08b42 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,18 +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);
Since this error gets masked, I was thinking it would would be better to change it to VIR_ERROR or even VIR_WARN, but ...
- return -1; + return 1; }
if (seclabel->label && parseIds(seclabel->label, &uid, &gid)) { - virReportError(VIR_ERR_INTERNAL_ERROR, + virReportError(VIR_ERR_INVALID_ARG, _("failed to parse uid and gid for DAC " "security driver: %s"), seclabel->label); return -1; @@ -127,8 +128,10 @@ static int virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv, uid_t *uidPtr, gid_t *gidPtr) { - if (virSecurityDACParseIds(def, uidPtr, gidPtr) == 0) - return 0; + int ret; + + if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0) + return ret;
if (priv) {
... in case this condition is false, the function returns -1 and there is no error to report. That can escalate up to starting the domain which will consequently fail without an error. The same applies for the image label below.
if (uidPtr) @@ -140,6 +143,8 @@ int virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv, return -1; }
+ +/* 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,19 +154,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; + return 1; }
if (seclabel->imagelabel && parseIds(seclabel->imagelabel, &uid, &gid)) { - virReportError(VIR_ERR_INTERNAL_ERROR, + virReportError(VIR_ERR_INVALID_ARG, _("failed to parse uid and gid for DAC " "security driver: %s"), seclabel->label); return -1; @@ -179,8 +184,10 @@ static int virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr priv, uid_t *uidPtr, gid_t *gidPtr) { - if (virSecurityDACParseImageIds(def, uidPtr, gidPtr) == 0) - return 0; + int ret; + + if ((ret = virSecurityDACParseImageIds(def, uidPtr, gidPtr)) <= 0) + return ret;
if (priv) { if (uidPtr)