Jan Safranek - 13:40 20.12.10 wrote:
> On 12/15/2010 08:18 PM, Michal Hrusecky wrote:
> > Rules can be written using wildcards, but destinations had to be static.
> > This patch adds support for following strings in destination:
> >
> > %u - uid
> > %U - username, uid in case of error
> > %g - gid
> > %G - group name, gid in case of error
> > %p - pid
> > %P - proccess name, pid in case of error
> >
> > More general rules can be specified using wildcards. Example rule can be:
> >
> > *...@users * %G/%U
> >
> > This will put all users in their own cgroups named by their login and
> > group.
> >
> > Signed-off-by: Michal Hrusecky<mic...@hrusecky.net>
> > Acked-by: Dhaval Giani<dhaval.gi...@gmail.com>
>
> Now it's much better, only one bug remains - see inline in the code.
>
> I do not like the style though, it's hard to read - there are too many
> indentation levels for me. Looking around the code, it seems that 'case'
> should be on the same level as 'switch', which saves you one tab and
> hopefully make the code nicer. And new function to augument group name
> would improve the readability a bit. Those are not strict requirements,
> if Dhaval agrees on current style (and it seems you already have his ack).
I was thinking about adding functions for group name and login
resolving, but in the end I would need to pass too many arguments inside
so I thought that it wouldn't help that much... Some macro that would
hide some parameters (destination and space available) might help but it
also can make it even worse. So in the end I just adjusted indentation.
> > ---
> > doc/man/cgrules.conf.5 | 11 ++++-
> > src/api.c | 109
> > +++++++++++++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 118 insertions(+), 2 deletions(-)
> >
> > diff --git a/doc/man/cgrules.conf.5 b/doc/man/cgrules.conf.5
> > index 4ca6937..9a8a0a3 100644
> > --- a/doc/man/cgrules.conf.5
> > +++ b/doc/man/cgrules.conf.5
> > @@ -57,7 +57,16 @@ can be:
> > can be:
> > .nf
> > - path relative to the controller hierarchy (ex. pgrp1/gid1/uid1)
> > -
> > + - following strings will get expanded
> > +
> > + %u username, uid if name resolving fails
> > + %U uid
> > + %g group name, gid if name resolving fails
> > + %G gid
> > + %p process name, pid if name not available
> > + %P pid
> > +
> > + '\\' can be used to escape '%'
> > .fi
> >
> > First rule which matches the criteria will be executed.
> > diff --git a/src/api.c b/src/api.c
> > index 859190a..ff45e08 100644
> > --- a/src/api.c
> > +++ b/src/api.c
> > @@ -2366,6 +2366,14 @@ int cgroup_change_cgroup_flags(uid_t uid, gid_t gid,
> > /* Temporary pointer to a rule */
> > struct cgroup_rule *tmp = NULL;
> >
> > + /* Temporary variables for destination substitution */
> > + char newdest[FILENAME_MAX];
> > + int i, j;
> > + int written;
> > + int available;
> > + struct passwd *user_info;
> > + struct group *group_info;
> > +
> > /* Return codes */
> > int ret = 0;
> >
> > @@ -2418,7 +2426,106 @@ int cgroup_change_cgroup_flags(uid_t uid, gid_t gid,
> > do {
> > cgroup_dbg("Executing rule %s for PID %d... ", tmp->username,
> > pid);
> > - ret = cgroup_change_cgroup_path(tmp->destination,
> > + /* Destination substitutions */
> > + for(j = i = 0; i< strlen(tmp->destination)&&
> > + (j< FILENAME_MAX - 2); ++i, ++j) {
> > + if(tmp->destination[i] == '%') {
> > + /* How many bytes did we write / error check */
> > + written = 0;
> > + /* How many bytes can we write */
> > + available = FILENAME_MAX - j - 2;
> > + /* Substitution */
> > + switch(tmp->destination[++i]) {
> > + case 'u':
> > + written = snprintf(newdest+j,
> > + available, "%d", uid);
> > + break;
> > + case 'U':
> > + user_info = getpwuid(uid);
> > + if(user_info) {
> > + written = snprintf(
> > + newdest + j,
> > + available,
> > + "%s",
> > + user_info ->
> > + pw_name);
>
> From snprintf man page: if the output was truncated due to this limit
> then the return value is the number of characters (not including the
> trailing '\0') which *would have been written* to the final string if
> enough space had been available.
>
> I.e. if strlen(pw_name) is bigger than 'available', snprintf returns
> full strlen(pw_name), not only length of the part which was copied.
> Which leads to problems later (see below)...
Overlooked that, thanks for pointing that out, fixed in the new version,
that I'm sending right now.
> > + } else {
> > + written = snprintf(
> > + newdest + j,
> > + available,
> > + "%d", uid);
> > + }
> > + break;
> > + case 'g':
> > + written = snprintf(newdest + j,
> > + available, "%d", gid);
> > + break;
> > + case 'G':
> > + group_info = getgrgid(gid);
> > + if(group_info) {
> > + written = snprintf(
> > + newdest + j,
> > + available,
> > + "%s",
> > + group_info ->
> > + gr_name);
> > + } else {
> > + written = snprintf(
> > + newdest + j,
> > + available,
> > + "%d", gid);
> > + }
> > + break;
> > + case 'p':
> > + written = snprintf(newdest + j,
> > + available, "%d", pid);
> > + break;
> > + case 'P':
> > + if(procname) {
> > + written = snprintf(
> > + newdest + j,
> > + available,
> > + "%s",
> > + procname);
> > + } else {
> > + written = snprintf(
> > + newdest + j,
> > + available,
> > + "%d", pid);
> > + }
> > + break;
> > + }
> > + /*
> > + * written<1 only when either error occurred
> > + * during snprintf or if no substitution was
> > + * made at all. In both cases, we want to just
> > + * copy input string.
> > + */
> > +
> > + if(written<1) {
> > + newdest[j] = '%';
> > + if(available>1)
> > + newdest[++j] =
> > + tmp->destination[i];
> > + } else {
> > + /*
> > + * In next iteration, we will write
> > + * just after the substitution, but j
> > + * will get incremented in the
> > + * meantime.
> > + */
> > + j += written - 1;
>
> As pointed above, 'written' can be quite big now and therefore 'j' can
> point *behind* the newdest[], see below...
>
> > + }
> > + } else {
> > + if(tmp->destination[i] == '\\')
> > + ++i;
> > + newdest[j] = tmp->destination[i];
> > + }
> > + }
> > + newdest[j] = 0;
>
> And here we have a nice buffer overflow :).
>
> > +
> > + /* Apply the rule */
> > + ret = cgroup_change_cgroup_path(newdest,
> > pid, (const char * const *)tmp->controllers);
> > if (ret) {
> > cgroup_dbg("FAILED! (Error Code: %d)\n", ret);
>
--
Michal Hrusecky <mic...@hrusecky.net>
------------------------------------------------------------------------------
Learn how Oracle Real Application Clusters (RAC) One Node allows customers
to consolidate database storage, standardize their database environment, and,
should the need arise, upgrade to a full multi-node Oracle RAC database
without downtime or disruption
http://p.sf.net/sfu/oracle-sfdevnl
_______________________________________________
Libcg-devel mailing list
Libcg-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libcg-devel
