9:52 p.m.
This patch series moves the logic for parsing users and groups in a
similar way to coreutils' chown from security_dac.c to util.c, as
suggested by Eric Blake.
This change has two majors side effects:
1. Some error messages that were issued when security_dac.c tried to
parse an ID as a name are no longer issued.
2. The keys `user` and `group` in qemu.conf can now be defined in the
same way that in DAC security labels.
Marcelo Cerri (2):
util: extend virGetUserID and virGetGroupID to support names and IDs
security: update user and group parsing in security_dac.c
src/security/security_dac.c | 40 ++++++---------------
src/util/util.c | 87 ++++++++++++++++++++++++++++++++++++++-------
2 files changed, 84 insertions(+), 43 deletions(-)
--
1.7.12
9:52 p.m.
New subject: [libvirt] [PATCH 1/2] util: extend virGetUserID and virGetGroupID to support names and IDs
This patch updates virGetUserID and virGetGroupID to be able to parse a
user or group name in a similar way to coreutils' chown. This means that
a numeric value with a leading plus sign is always parsed as an ID,
otherwise the functions try to parse the input first as a user or group
name and if this fails they try to parse it as an ID.
---
src/util/util.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 74 insertions(+), 13 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 43fdaf1..694ed3d 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -2495,9 +2495,11 @@ char *virGetGroupName(gid_t gid)
return virGetGroupEnt(gid);
}
-
-int virGetUserID(const char *name,
- uid_t *uid)
+/* Search in the password database for a user id that matches the user name
+ * `name`. Returns 0 on success, -1 on a critical failure or 1 if name cannot
+ * be parsed.
+ */
+static int virGetUserIDByName(const char *name, uid_t *uid)
{
char *strbuf;
struct passwd pwbuf;
@@ -2530,11 +2532,10 @@ int virGetUserID(const char *name,
}
}
if (rc != 0 || pw == NULL) {
- virReportSystemError(rc,
- _("Failed to find user record for name
'%s'"),
- name);
+ VIR_DEBUG("Failed to find user record for user '%s' (error =
%d)",
+ name, rc);
VIR_FREE(strbuf);
- return -1;
+ return 1;
}
*uid = pw->pw_uid;
@@ -2544,9 +2545,41 @@ int virGetUserID(const char *name,
return 0;
}
+/* Try to match a user id based on `user`. The default behavior is to parse
+ * `user` first as a user name and then as a user id. However if `user`
+ * contains a leading '+', the rest of the string is always parsed as a uid.
+ *
+ * Returns 0 on success and -1 otherwise.
+ */
+int virGetUserID(const char *user, uid_t *uid)
+{
+ unsigned int uint_uid;
-int virGetGroupID(const char *name,
- gid_t *gid)
+ if (*user == '+') {
+ user++;
+ } else {
+ int rc = virGetUserIDByName(user, uid);
+ if (rc <= 0)
+ return rc;
+ }
+
+ if (virStrToLong_ui(user, NULL, 10, &uint_uid) < 0 ||
+ ((uid_t) uint_uid) != uint_uid) {
+ virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user
'%s'"),
+ user);
+ return -1;
+ }
+
+ *uid = uint_uid;
+
+ return 0;
+}
+
+/* Search in the group database for a group id that matches the group name
+ * `name`. Returns 0 on success, -1 on a critical failure or 1 if name cannot
+ * be parsed.
+ */
+static int virGetGroupIDByName(const char *name, gid_t *gid)
{
char *strbuf;
struct group grbuf;
@@ -2579,11 +2612,10 @@ int virGetGroupID(const char *name,
}
}
if (rc != 0 || gr == NULL) {
- virReportSystemError(rc,
- _("Failed to find group record for name
'%s'"),
- name);
+ VIR_DEBUG("Failed to find group record for group '%s' (error =
%d)",
+ name, rc);
VIR_FREE(strbuf);
- return -1;
+ return 1;
}
*gid = gr->gr_gid;
@@ -2593,6 +2625,35 @@ int virGetGroupID(const char *name,
return 0;
}
+/* Try to match a group id based on `group`. The default behavior is to parse
+ * `group` first as a group name and then as a group id. However if `group`
+ * contains a leading '+', the rest of the string is always parsed as a guid.
+ *
+ * Returns 0 on success and -1 otherwise.
+ */
+int virGetGroupID(const char *group, gid_t *gid)
+{
+ unsigned int uint_gid;
+
+ if (*group == '+') {
+ group++;
+ } else {
+ int rc = virGetGroupIDByName(group, gid);
+ if (rc <= 0)
+ return rc;
+ }
+
+ if (virStrToLong_ui(group, NULL, 10, &uint_gid) < 0 ||
+ ((gid_t) uint_gid) != uint_gid) {
+ virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user
'%s'"),
+ group);
+ return -1;
+ }
+
+ *gid = uint_gid;
+
+ return 0;
+}
/* Set the real and effective uid and gid to the given values, and call
* initgroups so that the process has all the assumed group membership of
--
1.7.12
7:08 a.m.
New subject: [libvirt] [PATCH 1/2] util: extend virGetUserID and virGetGroupID to support names and IDs
On 10/06/12 04:52, Marcelo Cerri wrote:
This patch updates virGetUserID and virGetGroupID to be able to parse
a
user or group name in a similar way to coreutils' chown. This means that
a numeric value with a leading plus sign is always parsed as an ID,
otherwise the functions try to parse the input first as a user or group
name and if this fails they try to parse it as an ID.
---
src/util/util.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 74 insertions(+), 13 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 43fdaf1..694ed3d 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -2495,9 +2495,11 @@ char *virGetGroupName(gid_t gid)
return virGetGroupEnt(gid);
}
-
-int virGetUserID(const char *name,
- uid_t *uid)
+/* Search in the password database for a user id that matches the user name
+ * `name`. Returns 0 on success, -1 on a critical failure or 1 if name cannot
+ * be parsed.
+ */
+static int virGetUserIDByName(const char *name, uid_t *uid)
{
char *strbuf;
struct passwd pwbuf;
@@ -2530,11 +2532,10 @@ int virGetUserID(const char *name,
}
}
if (rc != 0 || pw == NULL) {
- virReportSystemError(rc,
- _("Failed to find user record for name
'%s'"),
- name);
+ VIR_DEBUG("Failed to find user record for user '%s' (error =
%d)",
+ name, rc);
Although this will work most of the times when an error occurs it will
be masked as if the user wasn't found. Unfortunately getpwuid_r and
friends have very bad error reporting. The only effective way to
distinguish errors from non-existent user entries (according to the
manpage) is to check set errno before the call and check it afterwards.
VIR_FREE(strbuf);
- return -1;
+ return 1;
}
*uid = pw->pw_uid;
@@ -2544,9 +2545,41 @@ int virGetUserID(const char *name,
return 0;
}
+/* Try to match a user id based on `user`. The default behavior is to parse
+ * `user` first as a user name and then as a user id. However if `user`
+ * contains a leading '+', the rest of the string is always parsed as a uid.
+ *
+ * Returns 0 on success and -1 otherwise.
+ */
+int virGetUserID(const char *user, uid_t *uid)
+{
+ unsigned int uint_uid;
-int virGetGroupID(const char *name,
- gid_t *gid)
+ if (*user == '+') {
+ user++;
+ } else {
+ int rc = virGetUserIDByName(user, uid);
+ if (rc <= 0)
+ return rc;
+ }
+
+ if (virStrToLong_ui(user, NULL, 10, &uint_uid) < 0 ||
+ ((uid_t) uint_uid) != uint_uid) {
+ virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user
'%s'"),
+ user);
+ return -1;
+ }
+
+ *uid = uint_uid;
+
+ return 0;
+}
+
Otherwise looks ok, so ACK. I'll push this after I get a review on the
followup patch that fixes the issue with errors being masked while
getting the user entry.
Peter
8:36 a.m.
New subject: [libvirt] [PATCH 1/2] util: extend virGetUserID and virGetGroupID to support names and IDs
On Mon, Oct 08, 2012 at 02:08:59PM +0200, Peter Krempa wrote:
On 10/06/12 04:52, Marcelo Cerri wrote:
>This patch updates virGetUserID and virGetGroupID to be able to parse a
>user or group name in a similar way to coreutils' chown. This means that
>a numeric value with a leading plus sign is always parsed as an ID,
>otherwise the functions try to parse the input first as a user or group
>name and if this fails they try to parse it as an ID.
>---
> src/util/util.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 74 insertions(+), 13 deletions(-)
>
>diff --git a/src/util/util.c b/src/util/util.c
>index 43fdaf1..694ed3d 100644
>--- a/src/util/util.c
>+++ b/src/util/util.c
>@@ -2495,9 +2495,11 @@ char *virGetGroupName(gid_t gid)
> return virGetGroupEnt(gid);
> }
>
>-
>-int virGetUserID(const char *name,
>- uid_t *uid)
>+/* Search in the password database for a user id that matches the user name
>+ * `name`. Returns 0 on success, -1 on a critical failure or 1 if name cannot
>+ * be parsed.
>+ */
>+static int virGetUserIDByName(const char *name, uid_t *uid)
> {
> char *strbuf;
> struct passwd pwbuf;
>@@ -2530,11 +2532,10 @@ int virGetUserID(const char *name,
> }
> }
> if (rc != 0 || pw == NULL) {
>- virReportSystemError(rc,
>- _("Failed to find user record for name
'%s'"),
>- name);
>+ VIR_DEBUG("Failed to find user record for user '%s' (error =
%d)",
>+ name, rc);
Although this will work most of the times when an error occurs it
will be masked as if the user wasn't found. Unfortunately getpwuid_r
and friends have very bad error reporting. The only effective way to
distinguish errors from non-existent user entries (according to the
manpage) is to check set errno before the call and check it
afterwards.
Do you think that I should keep virReportSystemError instead of
VIR_DEBUG here?
> VIR_FREE(strbuf);
>- return -1;
>+ return 1;
> }
>
> *uid = pw->pw_uid;
>@@ -2544,9 +2545,41 @@ int virGetUserID(const char *name,
> return 0;
> }
>
>+/* Try to match a user id based on `user`. The default behavior is to parse
>+ * `user` first as a user name and then as a user id. However if `user`
>+ * contains a leading '+', the rest of the string is always parsed as a
uid.
>+ *
>+ * Returns 0 on success and -1 otherwise.
>+ */
>+int virGetUserID(const char *user, uid_t *uid)
>+{
>+ unsigned int uint_uid;
>
>-int virGetGroupID(const char *name,
>- gid_t *gid)
>+ if (*user == '+') {
>+ user++;
>+ } else {
>+ int rc = virGetUserIDByName(user, uid);
>+ if (rc <= 0)
>+ return rc;
>+ }
>+
>+ if (virStrToLong_ui(user, NULL, 10, &uint_uid) < 0 ||
>+ ((uid_t) uint_uid) != uint_uid) {
>+ virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user
'%s'"),
>+ user);
>+ return -1;
>+ }
>+
>+ *uid = uint_uid;
>+
>+ return 0;
>+}
>+
Otherwise looks ok, so ACK. I'll push this after I get a review on
the followup patch that fixes the issue with errors being masked
while getting the user entry.
Peter
9:13 a.m.
New subject: [libvirt] [PATCH 1/2] util: extend virGetUserID and virGetGroupID to support names and IDs
On Mon, Oct 08, 2012 at 10:36:22AM -0300, Marcelo Cerri wrote:
On Mon, Oct 08, 2012 at 02:08:59PM +0200, Peter Krempa wrote:
> On 10/06/12 04:52, Marcelo Cerri wrote:
> >This patch updates virGetUserID and virGetGroupID to be able to parse a
> >user or group name in a similar way to coreutils' chown. This means that
> >a numeric value with a leading plus sign is always parsed as an ID,
> >otherwise the functions try to parse the input first as a user or group
> >name and if this fails they try to parse it as an ID.
> >---
> > src/util/util.c | 87
++++++++++++++++++++++++++++++++++++++++++++++++---------
> > 1 file changed, 74 insertions(+), 13 deletions(-)
> >
> >diff --git a/src/util/util.c b/src/util/util.c
> >index 43fdaf1..694ed3d 100644
> >--- a/src/util/util.c
> >+++ b/src/util/util.c
> >@@ -2495,9 +2495,11 @@ char *virGetGroupName(gid_t gid)
> > return virGetGroupEnt(gid);
> > }
> >
> >-
> >-int virGetUserID(const char *name,
> >- uid_t *uid)
> >+/* Search in the password database for a user id that matches the user name
> >+ * `name`. Returns 0 on success, -1 on a critical failure or 1 if name cannot
> >+ * be parsed.
> >+ */
> >+static int virGetUserIDByName(const char *name, uid_t *uid)
> > {
> > char *strbuf;
> > struct passwd pwbuf;
> >@@ -2530,11 +2532,10 @@ int virGetUserID(const char *name,
> > }
> > }
> > if (rc != 0 || pw == NULL) {
> >- virReportSystemError(rc,
> >- _("Failed to find user record for name
'%s'"),
> >- name);
> >+ VIR_DEBUG("Failed to find user record for user '%s' (error
= %d)",
> >+ name, rc);
>
> Although this will work most of the times when an error occurs it
> will be masked as if the user wasn't found. Unfortunately getpwuid_r
> and friends have very bad error reporting. The only effective way to
> distinguish errors from non-existent user entries (according to the
> manpage) is to check set errno before the call and check it
> afterwards.
Do you think that I should keep virReportSystemError instead of
VIR_DEBUG here?
Regarding getpwnam_r, I think that for the reentrant version the error
number is indicated by its return value, however it has the same issues
that getpwname has with errno.
As you said, one option is to differentiate an error from a name that
doesn't exist using this value, but the description in the man page is
not accurate:
ERRORS
0 or ENOENT or ESRCH or EBADF or EPERM or ...
The given name or uid was not found.
I could consider just ENOENT, ESRCH, EBADF and EPERM as non errors and
return 1 for them, but we might miss some other error numbers that also
indicate that the name was not found.
>
> > VIR_FREE(strbuf);
> >- return -1;
> >+ return 1;
> > }
> >
> > *uid = pw->pw_uid;
> >@@ -2544,9 +2545,41 @@ int virGetUserID(const char *name,
> > return 0;
> > }
> >
> >+/* Try to match a user id based on `user`. The default behavior is to parse
> >+ * `user` first as a user name and then as a user id. However if `user`
> >+ * contains a leading '+', the rest of the string is always parsed as a
uid.
> >+ *
> >+ * Returns 0 on success and -1 otherwise.
> >+ */
> >+int virGetUserID(const char *user, uid_t *uid)
> >+{
> >+ unsigned int uint_uid;
> >
> >-int virGetGroupID(const char *name,
> >- gid_t *gid)
> >+ if (*user == '+') {
> >+ user++;
> >+ } else {
> >+ int rc = virGetUserIDByName(user, uid);
> >+ if (rc <= 0)
> >+ return rc;
> >+ }
> >+
> >+ if (virStrToLong_ui(user, NULL, 10, &uint_uid) < 0 ||
> >+ ((uid_t) uint_uid) != uint_uid) {
> >+ virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user
'%s'"),
> >+ user);
> >+ return -1;
> >+ }
> >+
> >+ *uid = uint_uid;
> >+
> >+ return 0;
> >+}
> >+
>
> Otherwise looks ok, so ACK. I'll push this after I get a review on
> the followup patch that fixes the issue with errors being masked
> while getting the user entry.
>
> Peter
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
9:27 a.m.
New subject: [libvirt] [PATCH 1/2] util: extend virGetUserID and virGetGroupID to support names and IDs
On 10/08/12 16:13, Marcelo Cerri wrote:
On Mon, Oct 08, 2012 at 10:36:22AM -0300, Marcelo Cerri wrote:
> On Mon, Oct 08, 2012 at 02:08:59PM +0200, Peter Krempa wrote:
>> On 10/06/12 04:52, Marcelo Cerri wrote:
>>> This patch updates virGetUserID and virGetGroupID to be able to parse a
>>> user or group name in a similar way to coreutils' chown. This means that
>>> a numeric value with a leading plus sign is always parsed as an ID,
>>> otherwise the functions try to parse the input first as a user or group
>>> name and if this fails they try to parse it as an ID.
>>> ---
>>> src/util/util.c | 87
++++++++++++++++++++++++++++++++++++++++++++++++---------
>>> 1 file changed, 74 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/src/util/util.c b/src/util/util.c
>>> index 43fdaf1..694ed3d 100644
>>> --- a/src/util/util.c
>>> +++ b/src/util/util.c
>>> @@ -2495,9 +2495,11 @@ char *virGetGroupName(gid_t gid)
>>> return virGetGroupEnt(gid);
>>> }
>>>
>>> -
>>> -int virGetUserID(const char *name,
>>> - uid_t *uid)
>>> +/* Search in the password database for a user id that matches the user name
>>> + * `name`. Returns 0 on success, -1 on a critical failure or 1 if name
cannot
>>> + * be parsed.
>>> + */
>>> +static int virGetUserIDByName(const char *name, uid_t *uid)
>>> {
>>> char *strbuf;
>>> struct passwd pwbuf;
>>> @@ -2530,11 +2532,10 @@ int virGetUserID(const char *name,
>>> }
>>> }
>>> if (rc != 0 || pw == NULL) {
>>> - virReportSystemError(rc,
>>> - _("Failed to find user record for name
'%s'"),
>>> - name);
>>> + VIR_DEBUG("Failed to find user record for user '%s'
(error = %d)",
>>> + name, rc);
>>
>> Although this will work most of the times when an error occurs it
>> will be masked as if the user wasn't found. Unfortunately getpwuid_r
>> and friends have very bad error reporting. The only effective way to
>> distinguish errors from non-existent user entries (according to the
>> manpage) is to check set errno before the call and check it
>> afterwards.
>
> Do you think that I should keep virReportSystemError instead of
> VIR_DEBUG here?
Regarding getpwnam_r, I think that for the reentrant version the error
number is indicated by its return value, however it has the same issues
that getpwname has with errno.
As you said, one option is to differentiate an error from a name that
doesn't exist using this value, but the description in the man page is
not accurate:
ERRORS
0 or ENOENT or ESRCH or EBADF or EPERM or ...
The given name or uid was not found.
I could consider just ENOENT, ESRCH, EBADF and EPERM as non errors and
return 1 for them, but we might miss some other error numbers that also
indicate that the name was not found.
Actually, according to the man page, the reentrant version works as
expected:
When the return value is 0 the function succeeded and:
- if the pointer is NULL, the entry was not found
- if the pointer is non-null it's valid.
I sent a patch that fixes this issue:
http://www.redhat.com/archives/libvir-list/2012-October/msg00234.html
Peter
Peter
8:18 a.m.
New subject: [libvirt] [PATCH 1/2] util: extend virGetUserID and virGetGroupID to support names and IDs
On Fri, Oct 05, 2012 at 11:52:23PM -0300, Marcelo Cerri wrote:
This patch updates virGetUserID and virGetGroupID to be able to parse
a
user or group name in a similar way to coreutils' chown. This means that
a numeric value with a leading plus sign is always parsed as an ID,
otherwise the functions try to parse the input first as a user or group
name and if this fails they try to parse it as an ID.
---
src/util/util.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 74 insertions(+), 13 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 43fdaf1..694ed3d 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -2495,9 +2495,11 @@ char *virGetGroupName(gid_t gid)
return virGetGroupEnt(gid);
}
-
-int virGetUserID(const char *name,
- uid_t *uid)
+/* Search in the password database for a user id that matches the user name
+ * `name`. Returns 0 on success, -1 on a critical failure or 1 if name cannot
+ * be parsed.
+ */
+static int virGetUserIDByName(const char *name, uid_t *uid)
{
char *strbuf;
struct passwd pwbuf;
@@ -2530,11 +2532,10 @@ int virGetUserID(const char *name,
}
}
if (rc != 0 || pw == NULL) {
- virReportSystemError(rc,
- _("Failed to find user record for name
'%s'"),
- name);
+ VIR_DEBUG("Failed to find user record for user '%s' (error =
%d)",
+ name, rc);
VIR_FREE(strbuf);
- return -1;
+ return 1;
}
*uid = pw->pw_uid;
@@ -2544,9 +2545,41 @@ int virGetUserID(const char *name,
return 0;
}
+/* Try to match a user id based on `user`. The default behavior is to parse
+ * `user` first as a user name and then as a user id. However if `user`
+ * contains a leading '+', the rest of the string is always parsed as a uid.
+ *
+ * Returns 0 on success and -1 otherwise.
+ */
+int virGetUserID(const char *user, uid_t *uid)
+{
+ unsigned int uint_uid;
-int virGetGroupID(const char *name,
- gid_t *gid)
+ if (*user == '+') {
+ user++;
+ } else {
+ int rc = virGetUserIDByName(user, uid);
+ if (rc <= 0)
+ return rc;
+ }
+
+ if (virStrToLong_ui(user, NULL, 10, &uint_uid) < 0 ||
+ ((uid_t) uint_uid) != uint_uid) {
+ virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user
'%s'"),
+ user);
+ return -1;
+ }
+
+ *uid = uint_uid;
+
+ return 0;
+}
+
+/* Search in the group database for a group id that matches the group name
+ * `name`. Returns 0 on success, -1 on a critical failure or 1 if name cannot
+ * be parsed.
+ */
+static int virGetGroupIDByName(const char *name, gid_t *gid)
{
char *strbuf;
struct group grbuf;
@@ -2579,11 +2612,10 @@ int virGetGroupID(const char *name,
}
}
if (rc != 0 || gr == NULL) {
- virReportSystemError(rc,
- _("Failed to find group record for name
'%s'"),
- name);
+ VIR_DEBUG("Failed to find group record for group '%s' (error =
%d)",
+ name, rc);
VIR_FREE(strbuf);
- return -1;
+ return 1;
}
*gid = gr->gr_gid;
@@ -2593,6 +2625,35 @@ int virGetGroupID(const char *name,
return 0;
}
+/* Try to match a group id based on `group`. The default behavior is to parse
+ * `group` first as a group name and then as a group id. However if `group`
+ * contains a leading '+', the rest of the string is always parsed as a guid.
+ *
+ * Returns 0 on success and -1 otherwise.
+ */
+int virGetGroupID(const char *group, gid_t *gid)
+{
+ unsigned int uint_gid;
+
+ if (*group == '+') {
+ group++;
+ } else {
+ int rc = virGetGroupIDByName(group, gid);
+ if (rc <= 0)
+ return rc;
+ }
+
+ if (virStrToLong_ui(group, NULL, 10, &uint_gid) < 0 ||
+ ((gid_t) uint_gid) != uint_gid) {
+ virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user
'%s'"),
Hmmm.. There's a copy-and-paste problem here. It should be 'group'
instead of 'user'.
I will wait to check if any other fixes will be needed and include it in
a next version of this series.
+ group);
+ return -1;
+ }
+
+ *gid = uint_gid;
+
+ return 0;
+}
/* Set the real and effective uid and gid to the given values, and call
* initgroups so that the process has all the assumed group membership of
--
1.7.12
12:12 p.m.
New subject: [libvirt] [PATCH 1/2] util: extend virGetUserID and virGetGroupID to support names and IDs
On 10/05/2012 08:52 PM, Marcelo Cerri wrote:
This patch updates virGetUserID and virGetGroupID to be able to parse
a
user or group name in a similar way to coreutils' chown. This means that
a numeric value with a leading plus sign is always parsed as an ID,
otherwise the functions try to parse the input first as a user or group
name and if this fails they try to parse it as an ID.
---
src/util/util.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 74 insertions(+), 13 deletions(-)
-
-int virGetUserID(const char *name,
- uid_t *uid)
+/* Search in the password database for a user id that matches the user name
+ * `name`. Returns 0 on success, -1 on a critical failure or 1 if name cannot
+ * be parsed.
What's the difference between critical failure and inability to parse a
name, and how would a client use this distinction?
+ */
+static int virGetUserIDByName(const char *name, uid_t *uid)
As long as you are reformatting this, I'd follow our convention of
splitting the return value from the function name, so that the function
name begins in column 1:
static int
virGetUserIDByName(...)
{
char *strbuf;
struct passwd pwbuf;
@@ -2530,11 +2532,10 @@ int virGetUserID(const char *name,
}
}
if (rc != 0 || pw == NULL) {
- virReportSystemError(rc,
- _("Failed to find user record for name
'%s'"),
- name);
+ VIR_DEBUG("Failed to find user record for user '%s' (error =
%d)",
+ name, rc);
When rc is non-zero, we hit an error, and should use
virReportSystemError() to expose the errno value that explains the
failure. This part of the patch is wrong, as you have a regression in
the loss of a valid error message to the log; also, when rc is non-zero,
we should return -1.
On the other hand, when rc is zero bug pw is NULL, then the name lookup
failed. In this case, I can see why you wanted to return a new value
(1) to indicate that there is no name tied to this lookup, while
avoiding pollution of the logs (VIR_DEBUG being weaker than
virReportSystemError) - IF we are going to attempt something else later,
such as seeing if the name string is also a valid number.
I also see that Peter tried to patch along these same lines. I think it
would be helpful if you reposted the series with both yours and Peter's
improvements in a single series.
+/* Try to match a user id based on `user`. The default behavior is
to parse
+ * `user` first as a user name and then as a user id. However if `user`
+ * contains a leading '+', the rest of the string is always parsed as a uid.
+ *
+ * Returns 0 on success and -1 otherwise.
+ */
+int virGetUserID(const char *user, uid_t *uid)
+{
+ unsigned int uint_uid;
Are we sure that 'unsigned int' is large enough for uid_t on all
platforms? I'd feel safer with something like:
verify(sizeof(unsigned int) >= sizeof(uid_t));
added to enforce this with the compiler.
+ if (*user == '+') {
+ user++;
+ } else {
+ int rc = virGetUserIDByName(user, uid);
+ if (rc <= 0)
+ return rc;
+ }
+
+ if (virStrToLong_ui(user, NULL, 10, &uint_uid) < 0 ||
+ ((uid_t) uint_uid) != uint_uid) {
+ virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user
'%s'"),
+ user);
Okay, so you DO report an error if the name lookup failed, and the
string is still numeric; while avoiding a log message if the name lookup
failed but a number works instead.
+ return -1;
+ }
+
+ *uid = uint_uid;
You are missing a check for overflow - on systems with 16-bit uid_t but
32-bit unsigned int, you need to make sure this assignment doesn't
change the value.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:44 p.m.
New subject: [libvirt] [PATCH 1/2] util: extend virGetUserID and virGetGroupID to support names and IDs
On Mon, Oct 08, 2012 at 11:12:49AM -0600, Eric Blake wrote:
On 10/05/2012 08:52 PM, Marcelo Cerri wrote:
> This patch updates virGetUserID and virGetGroupID to be able to parse a
> user or group name in a similar way to coreutils' chown. This means that
> a numeric value with a leading plus sign is always parsed as an ID,
> otherwise the functions try to parse the input first as a user or group
> name and if this fails they try to parse it as an ID.
> ---
> src/util/util.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 74 insertions(+), 13 deletions(-)
>
>
> -
> -int virGetUserID(const char *name,
> - uid_t *uid)
> +/* Search in the password database for a user id that matches the user name
> + * `name`. Returns 0 on success, -1 on a critical failure or 1 if name cannot
> + * be parsed.
What's the difference between critical failure and inability to parse a
name, and how would a client use this distinction?
This comment is actually wrong... It should be:
/* Search in the password database for a user id that matches the user name
* `name`. Returns 0 on success, -1 on failure or 1 if name cannot be found.
Are you ok with this kind of return?
> + */
> +static int virGetUserIDByName(const char *name, uid_t *uid)
As long as you are reformatting this, I'd follow our convention of
splitting the return value from the function name, so that the function
name begins in column 1:
static int
virGetUserIDByName(...)
Ok, no problem.
> {
> char *strbuf;
> struct passwd pwbuf;
> @@ -2530,11 +2532,10 @@ int virGetUserID(const char *name,
> }
> }
> if (rc != 0 || pw == NULL) {
> - virReportSystemError(rc,
> - _("Failed to find user record for name
'%s'"),
> - name);
> + VIR_DEBUG("Failed to find user record for user '%s' (error =
%d)",
> + name, rc);
When rc is non-zero, we hit an error, and should use
virReportSystemError() to expose the errno value that explains the
failure. This part of the patch is wrong, as you have a regression in
the loss of a valid error message to the log; also, when rc is non-zero,
we should return -1.
On the other hand, when rc is zero bug pw is NULL, then the name lookup
failed. In this case, I can see why you wanted to return a new value
(1) to indicate that there is no name tied to this lookup, while
avoiding pollution of the logs (VIR_DEBUG being weaker than
virReportSystemError) - IF we are going to attempt something else later,
such as seeing if the name string is also a valid number.
Yes, I was trying to avoid log pollution without depending on the error
number returned, but...
I also see that Peter tried to patch along these same lines. I think it
would be helpful if you reposted the series with both yours and Peter's
improvements in a single series.
as Peter has noticed, the reentrant versions of these functions return a
consistent value indicating that a name doesn't exist or a error has
occurred.
I'll repost this series including Peter's patch.
> +/* Try to match a user id based on `user`. The default behavior is to parse
> + * `user` first as a user name and then as a user id. However if `user`
> + * contains a leading '+', the rest of the string is always parsed as a
uid.
> + *
> + * Returns 0 on success and -1 otherwise.
> + */
> +int virGetUserID(const char *user, uid_t *uid)
> +{
> + unsigned int uint_uid;
Are we sure that 'unsigned int' is large enough for uid_t on all
platforms? I'd feel safer with something like:
verify(sizeof(unsigned int) >= sizeof(uid_t));
added to enforce this with the compiler.
> + if (*user == '+') {
> + user++;
> + } else {
> + int rc = virGetUserIDByName(user, uid);
> + if (rc <= 0)
> + return rc;
> + }
> +
> + if (virStrToLong_ui(user, NULL, 10, &uint_uid) < 0 ||
[1] I'm doing overflow check here. I'm reporting the same error for
overflow and when it cannot be parsed as a numeric value. Do you think
these two errors should be reported separately?
> + ((uid_t) uint_uid) != uint_uid) {
> + virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user
'%s'"),
> + user);
Okay, so you DO report an error if the name lookup failed, and the
string is still numeric; while avoiding a log message if the name lookup
failed but a number works instead.
> + return -1;
> + }
> +
> + *uid = uint_uid;
You are missing a check for overflow - on systems with 16-bit uid_t but
32-bit unsigned int, you need to make sure this assignment doesn't
change the value.
I'm doing that in [1].
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:57 p.m.
New subject: [libvirt] [PATCH 1/2] util: extend virGetUserID and virGetGroupID to support names and IDs
On 10/08/2012 11:44 AM, Marcelo Cerri wrote:
This comment is actually wrong... It should be:
/* Search in the password database for a user id that matches the user name
* `name`. Returns 0 on success, -1 on failure or 1 if name cannot be found.
Are you ok with this kind of return?
For a helper function, yes, that works.
Wondering aloud: as long as you are writing a helper function, does it
make sense to try and merge user and group parsing into a single
function, where you pass a function pointer (which will be either
getpwnam_r or getgrnam_r), or do we run into too many issues with
properly wording error messages?
> When rc is non-zero, we hit an error, and should use
> virReportSystemError() to expose the errno value that explains the
> failure. This part of the patch is wrong, as you have a regression in
> the loss of a valid error message to the log; also, when rc is non-zero,
> we should return -1.
The other alternative is to do NO error reporting in the helper function
(just VIR_DEBUG), return -errno on failure, 0 on success, and 1 on
lookup, and have the caller do the appropriate error reporting based on
the return value, where the caller then knows whether it was "user" or
"group" that failed.
>> + if (virStrToLong_ui(user, NULL, 10, &uint_uid) <
0 ||
[1] I'm doing overflow check here. I'm reporting the same error for
overflow and when it cannot be parsed as a numeric value. Do you think
these two errors should be reported separately?
There's two cases where overflow can occur. For example, when uid_t is
16 bits,
'+65536' will be an overflow if uid_t is 16 bits
'65536' might be a valid user name, but if the user name lookup fails,
then it is not valid as a uid_t
Reporting the error for both strings as "failed to parse user '%s'"
seems sufficient; I'm not sure it's worth the extra work to distinguish
between unable to parse vs. able to parse as a number but it didn't fit
in uid_t, since most users won't trigger either failure.
>> + ((uid_t) uint_uid) != uint_uid) {
>> + virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user
'%s'"),
>> + user);
So I guess I missed your overflow check, and you are doing this part
correctly after all. I'll be more careful in my review of v2 :)
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:04 p.m.
New subject: [libvirt] [PATCH 1/2] util: extend virGetUserID and virGetGroupID to support names and IDs
On 10/08/12 19:57, Eric Blake wrote:
Peter
On 10/08/2012 11:44 AM, Marcelo Cerri wrote:
> This comment is actually wrong... It should be:
> /* Search in the password database for a user id that
matches the user name
> * `name`. Returns 0 on success, -1 on failure or 1 if name cannot be found.
> Are you ok with this kind of return?
For a helper function, yes, that works.
Wondering aloud: as long as you are writing a helper function, does it
make sense to try and merge user and group parsing into a single
function, where you pass a function pointer (which will be either
getpwnam_r or getgrnam_r), or do we run into too many issues with
properly wording error messages?
I was wondering this myself when reviewing the original patchset. The
problem with merging those is different type of the arguments ("struct
passwd" vs "struct group") for those functions. I evaluated it as it's
not worth spending the effort of trying to combine them.
>> When rc is non-zero, we hit an error, and should use
>> virReportSystemError() to expose the errno value that explains the
>> failure. This part of the patch is wrong, as you have a regression in
>> the loss of a valid error message to the log; also, when rc is non-zero,
>> we should return -1.
The other alternative is to do NO error reporting in the helper function
(just VIR_DEBUG), return -errno on failure, 0 on success, and 1 on
lookup, and have the caller do the appropriate error reporting based on
the return value, where the caller then knows whether it was "user" or
"group" that failed.
This idea also seems reasonable (although we probably can't combine them).
1:25 p.m.
New subject: [libvirt] [PATCH 1/2] util: extend virGetUserID and virGetGroupID to support names and IDs
On Mon, Oct 08, 2012 at 08:04:53PM +0200, Peter Krempa wrote:
On 10/08/12 19:57, Eric Blake wrote:
>On 10/08/2012 11:44 AM, Marcelo Cerri wrote:
>
>>This comment is actually wrong... It should be:
>>
>>/* Search in the password database for a user id that matches the user name
>> * `name`. Returns 0 on success, -1 on failure or 1 if name cannot be found.
>>
>>Are you ok with this kind of return?
>
>For a helper function, yes, that works.
>
>Wondering aloud: as long as you are writing a helper function, does it
>make sense to try and merge user and group parsing into a single
>function, where you pass a function pointer (which will be either
>getpwnam_r or getgrnam_r), or do we run into too many issues with
>properly wording error messages?
I was wondering this myself when reviewing the original patchset.
The problem with merging those is different type of the arguments
("struct passwd" vs "struct group") for those functions. I evaluated
it as it's not worth spending the effort of trying to combine them.
I was tempted to do that. coreutils uses a generic approach for it and
makes extensive use of macros for this. But I agree with Peter, I don't
think it's worth the effort and we would end up with code much more
complex and hard to maintain.
>
>>>When rc is non-zero, we hit an error, and should use
>>>virReportSystemError() to expose the errno value that explains the
>>>failure. This part of the patch is wrong, as you have a regression in
>>>the loss of a valid error message to the log; also, when rc is non-zero,
>>>we should return -1.
>
>The other alternative is to do NO error reporting in the helper function
>(just VIR_DEBUG), return -errno on failure, 0 on success, and 1 on
>lookup, and have the caller do the appropriate error reporting based on
>the return value, where the caller then knows whether it was "user" or
>"group" that failed.
This idea also seems reasonable (although we probably can't combine them).
I would go with it if we just returned errors from get{pw,gr}nam_r, but
since we also return error for OOM, I vote for keeping it as it is.
>
Peter
2:16 p.m.
New subject: [libvirt] [PATCH 1/2] util: extend virGetUserID and virGetGroupID to support names and IDs
On Mon, Oct 08, 2012 at 11:12:49AM -0600, Eric Blake wrote:
On 10/05/2012 08:52 PM, Marcelo Cerri wrote:
> This patch updates virGetUserID and virGetGroupID to be able to parse a
> user or group name in a similar way to coreutils' chown. This means that
> a numeric value with a leading plus sign is always parsed as an ID,
> otherwise the functions try to parse the input first as a user or group
> name and if this fails they try to parse it as an ID.
> ---
> src/util/util.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 74 insertions(+), 13 deletions(-)
>
>
> -
> -int virGetUserID(const char *name,
> - uid_t *uid)
> +/* Search in the password database for a user id that matches the user name
> + * `name`. Returns 0 on success, -1 on a critical failure or 1 if name cannot
> + * be parsed.
What's the difference between critical failure and inability to parse a
name, and how would a client use this distinction?
> + */
> +static int virGetUserIDByName(const char *name, uid_t *uid)
As long as you are reformatting this, I'd follow our convention of
splitting the return value from the function name, so that the function
name begins in column 1:
static int
virGetUserIDByName(...)
> {
> char *strbuf;
> struct passwd pwbuf;
> @@ -2530,11 +2532,10 @@ int virGetUserID(const char *name,
> }
> }
> if (rc != 0 || pw == NULL) {
> - virReportSystemError(rc,
> - _("Failed to find user record for name
'%s'"),
> - name);
> + VIR_DEBUG("Failed to find user record for user '%s' (error =
%d)",
> + name, rc);
When rc is non-zero, we hit an error, and should use
virReportSystemError() to expose the errno value that explains the
failure. This part of the patch is wrong, as you have a regression in
the loss of a valid error message to the log; also, when rc is non-zero,
we should return -1.
On the other hand, when rc is zero bug pw is NULL, then the name lookup
failed. In this case, I can see why you wanted to return a new value
(1) to indicate that there is no name tied to this lookup, while
avoiding pollution of the logs (VIR_DEBUG being weaker than
virReportSystemError) - IF we are going to attempt something else later,
such as seeing if the name string is also a valid number.
I also see that Peter tried to patch along these same lines. I think it
would be helpful if you reposted the series with both yours and Peter's
improvements in a single series.
> +/* Try to match a user id based on `user`. The default behavior is to parse
> + * `user` first as a user name and then as a user id. However if `user`
> + * contains a leading '+', the rest of the string is always parsed as a
uid.
> + *
> + * Returns 0 on success and -1 otherwise.
> + */
> +int virGetUserID(const char *user, uid_t *uid)
> +{
> + unsigned int uint_uid;
Are we sure that 'unsigned int' is large enough for uid_t on all
platforms? I'd feel safer with something like:
verify(sizeof(unsigned int) >= sizeof(uid_t));
added to enforce this with the compiler.
Hi Eric, just a note: this seems to be already added by you :)
http://libvirt.org/git/?p=libvirt.git;a=commit;h=7f31e28c6efa3aba17f1e6ee...
> + if (*user == '+') {
> + user++;
> + } else {
> + int rc = virGetUserIDByName(user, uid);
> + if (rc <= 0)
> + return rc;
> + }
> +
> + if (virStrToLong_ui(user, NULL, 10, &uint_uid) < 0 ||
> + ((uid_t) uint_uid) != uint_uid) {
> + virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user
'%s'"),
> + user);
Okay, so you DO report an error if the name lookup failed, and the
string is still numeric; while avoiding a log message if the name lookup
failed but a number works instead.
> + return -1;
> + }
> +
> + *uid = uint_uid;
You are missing a check for overflow - on systems with 16-bit uid_t but
32-bit unsigned int, you need to make sure this assignment doesn't
change the value.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:49 p.m.
New subject: [libvirt] [PATCH 1/2] util: extend virGetUserID and virGetGroupID to support names and IDs
On 10/08/2012 01:16 PM, Marcelo Cerri wrote:
>> +int virGetUserID(const char *user, uid_t *uid)
>> +{
>> + unsigned int uint_uid;
>
> Are we sure that 'unsigned int' is large enough for uid_t on all
> platforms? I'd feel safer with something like:
>
> verify(sizeof(unsigned int) >= sizeof(uid_t));
>
> added to enforce this with the compiler.
Hi Eric, just a note: this seems to be already added by you :)
http://libvirt.org/git/?p=libvirt.git;a=commit;h=7f31e28c6efa3aba17f1e6ee...
Good, this aspect is taken care of then :)
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:52 p.m.
New subject: [libvirt] [PATCH 2/2] security: update user and group parsing in security_dac.c
The functions virGetUserID and virGetGroupID are now able to parse
user/group names and IDs in a similar way to coreutils' chown. So, user
and group parsing in security_dac can be simplified.
---
src/security/security_dac.c | 40 ++++++++++------------------------------
1 file changed, 10 insertions(+), 30 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index a427e9d..e976144 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -95,39 +95,19 @@ int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
group = sep + 1;
/* Parse owner */
- if (*owner == '+') {
- if (virStrToLong_ui(++owner, NULL, 10, &theuid) < 0) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("Invalid uid \"%s\" in DAC label
\"%s\""),
- owner, label);
- goto cleanup;
- }
- } else {
- if (virGetUserID(owner, &theuid) < 0 &&
- virStrToLong_ui(owner, NULL, 10, &theuid) < 0) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("Invalid owner \"%s\" in DAC label
\"%s\""),
- owner, label);
- goto cleanup;
- }
+ if (virGetUserID(owner, &theuid) < 0) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("Invalid owner \"%s\" in DAC label
\"%s\""),
+ owner, label);
+ goto cleanup;
}
/* Parse group */
- if (*group == '+') {
- if (virStrToLong_ui(++group, NULL, 10, &thegid) < 0) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("Invalid gid \"%s\" in DAC label
\"%s\""),
- group, label);
- goto cleanup;
- }
- } else {
- if (virGetGroupID(group, &thegid) < 0 &&
- virStrToLong_ui(group, NULL, 10, &thegid) < 0) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("Invalid group \"%s\" in DAC label
\"%s\""),
- group, label);
- goto cleanup;
- }
+ if (virGetGroupID(group, &thegid) < 0) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("Invalid group \"%s\" in DAC label
\"%s\""),
+ group, label);
+ goto cleanup;
}
if (uidPtr)
--
1.7.12
7:11 a.m.
New subject: [libvirt] [PATCH 2/2] security: update user and group parsing in security_dac.c
On 10/06/12 04:52, Marcelo Cerri wrote:
The functions virGetUserID and virGetGroupID are now able to parse
user/group names and IDs in a similar way to coreutils' chown. So, user
and group parsing in security_dac can be simplified.
---
src/security/security_dac.c | 40 ++++++++++------------------------------
1 file changed, 10 insertions(+), 30 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index a427e9d..e976144 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -95,39 +95,19 @@ int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
group = sep + 1;
/* Parse owner */
- if (*owner == '+') {
- if (virStrToLong_ui(++owner, NULL, 10, &theuid) < 0) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("Invalid uid \"%s\" in DAC label
\"%s\""),
- owner, label);
- goto cleanup;
- }
- } else {
- if (virGetUserID(owner, &theuid) < 0 &&
- virStrToLong_ui(owner, NULL, 10, &theuid) < 0) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("Invalid owner \"%s\" in DAC label
\"%s\""),
- owner, label);
- goto cleanup;
- }
+ if (virGetUserID(owner, &theuid) < 0) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("Invalid owner \"%s\" in DAC label
\"%s\""),
+ owner, label);
+ goto cleanup;
}
/* Parse group */
- if (*group == '+') {
- if (virStrToLong_ui(++group, NULL, 10, &thegid) < 0) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("Invalid gid \"%s\" in DAC label
\"%s\""),
- group, label);
- goto cleanup;
- }
- } else {
- if (virGetGroupID(group, &thegid) < 0 &&
- virStrToLong_ui(group, NULL, 10, &thegid) < 0) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("Invalid group \"%s\" in DAC label
\"%s\""),
- group, label);
- goto cleanup;
- }
+ if (virGetGroupID(group, &thegid) < 0) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("Invalid group \"%s\" in DAC label
\"%s\""),
+ group, label);
Hm, this will mask the errors from virGetGroupID that might be useful.
Should we remove this message in favor of the underlying messages or
have this that has more relevant information where to find the issue but
maybe mask the reason?
Any opinions?
+ goto cleanup;
}
if (uidPtr)
Peter
8:52 a.m.
New subject: [libvirt] [PATCH 2/2] security: update user and group parsing in security_dac.c
On Mon, Oct 08, 2012 at 02:11:26PM +0200, Peter Krempa wrote:
On 10/06/12 04:52, Marcelo Cerri wrote:
>The functions virGetUserID and virGetGroupID are now able to parse
>user/group names and IDs in a similar way to coreutils' chown. So, user
>and group parsing in security_dac can be simplified.
>---
> src/security/security_dac.c | 40 ++++++++++------------------------------
> 1 file changed, 10 insertions(+), 30 deletions(-)
>
>diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>index a427e9d..e976144 100644
>--- a/src/security/security_dac.c
>+++ b/src/security/security_dac.c
>@@ -95,39 +95,19 @@ int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
> group = sep + 1;
>
> /* Parse owner */
>- if (*owner == '+') {
>- if (virStrToLong_ui(++owner, NULL, 10, &theuid) < 0) {
>- virReportError(VIR_ERR_INVALID_ARG,
>- _("Invalid uid \"%s\" in DAC label
\"%s\""),
>- owner, label);
>- goto cleanup;
>- }
>- } else {
>- if (virGetUserID(owner, &theuid) < 0 &&
>- virStrToLong_ui(owner, NULL, 10, &theuid) < 0) {
>- virReportError(VIR_ERR_INVALID_ARG,
>- _("Invalid owner \"%s\" in DAC label
\"%s\""),
>- owner, label);
>- goto cleanup;
>- }
>+ if (virGetUserID(owner, &theuid) < 0) {
>+ virReportError(VIR_ERR_INVALID_ARG,
>+ _("Invalid owner \"%s\" in DAC label
\"%s\""),
>+ owner, label);
>+ goto cleanup;
> }
>
> /* Parse group */
>- if (*group == '+') {
>- if (virStrToLong_ui(++group, NULL, 10, &thegid) < 0) {
>- virReportError(VIR_ERR_INVALID_ARG,
>- _("Invalid gid \"%s\" in DAC label
\"%s\""),
>- group, label);
>- goto cleanup;
>- }
>- } else {
>- if (virGetGroupID(group, &thegid) < 0 &&
>- virStrToLong_ui(group, NULL, 10, &thegid) < 0) {
>- virReportError(VIR_ERR_INVALID_ARG,
>- _("Invalid group \"%s\" in DAC label
\"%s\""),
>- group, label);
>- goto cleanup;
>- }
>+ if (virGetGroupID(group, &thegid) < 0) {
>+ virReportError(VIR_ERR_INVALID_ARG,
>+ _("Invalid group \"%s\" in DAC label
\"%s\""),
>+ group, label);
Hm, this will mask the errors from virGetGroupID that might be
useful. Should we remove this message in favor of the underlying
messages or have this that has more relevant information where to
find the issue but maybe mask the reason?
I thought about that when I was testing these changes and it seemed more
useful for me when the message points to where the problem is, in this
case the DAC label.
But you are right, it will mask the reason why a DAC label couldn't be
parsed.
Any opinions?
Maybe a solution with a nested error would be an alternative, what do
think? Not sure if it is a good idea and I don't know what is the best
way to implement it.
Here is an example of what I'm talking:
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index e976144..4f097cc 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -95,18 +95,24 @@ int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
group = sep + 1;
/* Parse owner */
+ virResetLastError();
if (virGetUserID(owner, &theuid) < 0) {
+ virErrorPtr nested_error = virGetLastError();
virReportError(VIR_ERR_INVALID_ARG,
- _("Invalid owner \"%s\" in DAC label
\"%s\""),
- owner, label);
+ _("Invalid owner \"%s\" in DAC label
\"%s\"%s%s"),
+ owner, label, nested_error ? ": " : "",
+ nested_error ? nested_error->message : "");
goto cleanup;
}
/* Parse group */
+ virResetLastError();
if (virGetGroupID(group, &thegid) < 0) {
+ virErrorPtr nested_error = virGetLastError();
virReportError(VIR_ERR_INVALID_ARG,
- _("Invalid group \"%s\" in DAC label
\"%s\""),
- group, label);
+ _("Invalid group \"%s\" in DAC label
\"%s\"%s%s"),
+ group, label, nested_error ? ": " : "",
+ nested_error ? nested_error->message : "");
goto cleanup;
}
--
1.7.12
>+ goto cleanup;
> }
>
> if (uidPtr)
>
Peter
12:22 p.m.
New subject: [libvirt] [PATCH 2/2] security: update user and group parsing in security_dac.c
On 10/08/2012 07:52 AM, Marcelo Cerri wrote:
On Mon, Oct 08, 2012 at 02:11:26PM +0200, Peter Krempa wrote:
> On 10/06/12 04:52, Marcelo Cerri wrote:
>> The functions virGetUserID and virGetGroupID are now able to parse
>> user/group names and IDs in a similar way to coreutils' chown. So, user
>> and group parsing in security_dac can be simplified.
>> ---
>> src/security/security_dac.c | 40 ++++++++++------------------------------
>> 1 file changed, 10 insertions(+), 30 deletions(-)
>>
>> + if (virGetGroupID(group, &thegid) < 0) {
>> + virReportError(VIR_ERR_INVALID_ARG,
>> + _("Invalid group \"%s\" in DAC label
\"%s\""),
>> + group, label);
>
> Hm, this will mask the errors from virGetGroupID that might be
> useful. Should we remove this message in favor of the underlying
> messages or have this that has more relevant information where to
> find the issue but maybe mask the reason?
I thought about that when I was testing these changes and it seemed more
useful for me when the message points to where the problem is, in this
case the DAC label.
But you are right, it will mask the reason why a DAC label couldn't be
parsed.
>
> Any opinions?
I think it will be pretty obvious from the virGetGroupID() error message
that the failure came from inability to parse a group id string, even if
we don't pinpoint which DAC label contained that string. I think it's
simpler to just reuse the already-present error than to attempt nesting
the messages.
Maybe a solution with a nested error would be an alternative, what do
think? Not sure if it is a good idea and I don't know what is the best
way to implement it.
Here is an example of what I'm talking:
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index e976144..4f097cc 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -95,18 +95,24 @@ int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
group = sep + 1;
/* Parse owner */
+ virResetLastError();
This line isn't necessary, since you either don't look at any previous
error, or you are guaranteed that virGetUserID overwrote any previous error.
if (virGetUserID(owner, &theuid) < 0) {
+ virErrorPtr nested_error = virGetLastError();
virReportError(VIR_ERR_INVALID_ARG,
- _("Invalid owner \"%s\" in DAC label
\"%s\""),
- owner, label);
+ _("Invalid owner \"%s\" in DAC label
\"%s\"%s%s"),
+ owner, label, nested_error ? ": " : "",
+ nested_error ? nested_error->message : "");
Trailing whitespace.
Yes, this would form a nested error message, if we thought that it helps
users. But I'm thinking it's a bit over the top, and that we're
probably okay doing just:
if (virGetUserID(owner, &theuid) < 0)
goto cleanup;
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:37 p.m.
New subject: [libvirt] [PATCH 2/2] security: update user and group parsing in security_dac.c
On 10/08/12 19:22, Eric Blake wrote:
On 10/08/2012 07:52 AM, Marcelo Cerri wrote:
> On Mon, Oct 08, 2012 at 02:11:26PM +0200, Peter Krempa wrote:
>> On 10/06/12 04:52, Marcelo Cerri wrote:
>>> The functions virGetUserID and virGetGroupID are now able to parse
>>> user/group names and IDs in a similar way to coreutils' chown. So, user
>>> and group parsing in security_dac can be simplified.
>>> ---
>>> src/security/security_dac.c | 40 ++++++++++------------------------------
>>> 1 file changed, 10 insertions(+), 30 deletions(-)
>>>
>>> + if (virGetGroupID(group, &thegid) < 0) {
>>> + virReportError(VIR_ERR_INVALID_ARG,
>>> + _("Invalid group \"%s\" in DAC label
\"%s\""),
>>> + group, label);
>>
>> Hm, this will mask the errors from virGetGroupID that might be
>> useful. Should we remove this message in favor of the underlying
>> messages or have this that has more relevant information where to
>> find the issue but maybe mask the reason?
>
> I thought about that when I was testing these changes and it seemed more
> useful for me when the message points to where the problem is, in this
> case the DAC label.
>
> But you are right, it will mask the reason why a DAC label couldn't be
> parsed.
>
>>
>> Any opinions?
I think it will be pretty obvious from the virGetGroupID() error message
that the failure came from inability to parse a group id string, even if
we don't pinpoint which DAC label contained that string. I think it's
simpler to just reuse the already-present error than to attempt nesting
the messages.
I agree, the nesting doesn't look good. (Too bad we don't have native
support for error message stacks.) I think it could be slightly more
helpful to state that the DAC label contains the problem but not at the
cost of masking the real error from beneath.
Peter
4429
days inactive
4431
days old
18 comments
3 participants
participants (3)
-
Eric Blake -
Marcelo Cerri -
Peter Krempa