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).
> ---
> 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)...
> + } 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);
------------------------------------------------------------------------------
Lotusphere 2011
Register now for Lotusphere 2011 and learn how
to connect the dots, take your collaborative environment
to the next level, and enter the era of Social Business.
http://p.sf.net/sfu/lotusphere-d2d
_______________________________________________
Libcg-devel mailing list
Libcg-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libcg-devel
