Jan Safranek - 14:18 15.12.10 wrote:
> On 12/15/2010 10:12 AM, 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
> >
> > So 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.
> 
> I really like the idea, would you mind updating man page(s) as well?

Sure, updated man will be included in next round.
 
> >
> > Signed-off-by: Michal Hrusecky<mic...@hrusecky.net>
> > ---
> >   src/api.c |   82 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >   1 files changed, 81 insertions(+), 1 deletions(-)
> >
> > diff --git a/src/api.c b/src/api.c
> > index 859190a..29b8627 100644
> > --- a/src/api.c
> > +++ b/src/api.c
> > @@ -2366,6 +2366,12 @@ 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;
> > +   struct passwd * user_info;
> > +   struct group  * group_info;
> > +
> >     /* Return codes */
> >     int ret = 0;
> >
> > @@ -2418,7 +2424,81 @@ 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); i++, j++) {
> > +                   if(tmp->destination[i] == '%') {
> > +                           switch(tmp->destination[++i]) {
> > +                                   case 'u':
> > +                                           j += snprintf(newdest+j,
> > +                                                   FILENAME_MAX-j,
> > +                                                   "%d", uid);
> > +                                           i++;
> > +                                           break;
> > +                                   case 'U':
> > +                                           user_info = getpwuid(uid);
> > +                                           if(user_info) {
> > +                                                   j += snprintf(newdest+j,
> > +                                                           FILENAME_MAX-j,
> > +                                                           "%s",
> > +                                                           user_info ->
> > +                                                           pw_name);
> > +                                           } else {
> > +                                                   j += snprintf(newdest+j,
> > +                                                           FILENAME_MAX-j,
> > +                                                           "%d", uid);
> > +                                           }
> > +                                           i++;
> > +                                           break;
> > +                                   case 'g':
> > +                                           j += snprintf(newdest+j,
> > +                                                   FILENAME_MAX-j,
> > +                                                   "%d", gid);
> > +                                           i++;
> > +                                           break;
> > +                                   case 'G':
> > +                                           group_info = getgrgid(gid);
> > +                                           if(group_info) {
> > +                                                   j += snprintf(newdest+j,
> > +                                                           FILENAME_MAX-j,
> > +                                                           "%s",
> > +                                                           group_info ->
> > +                                                           gr_name);
> > +                                           } else {
> > +                                                   j += snprintf(newdest+j,
> > +                                                           FILENAME_MAX-j,
> > +                                                           "%d", gid);
> > +                                           }
> > +                                           i++;
> > +                                           break;
> > +                                   case 'p':
> > +                                           j += snprintf(newdest+j,
> > +                                                   FILENAME_MAX-j,
> > +                                                   "%d", pid);
> > +                                           i++;
> > +                                           break;
> > +                                   case 'P':
> > +                                           if(procname) {
> > +                                                   j += snprintf(newdest+j,
> > +                                                           FILENAME_MAX-j,
> > +                                                           "%s",
> > +                                                           procname);
> > +                                           } else {
> > +                                                   j += snprintf(newdest+j,
> > +                                                           FILENAME_MAX-j,
> > +                                                           "%d", pid);
> > +                                           }
> > +                                           i++;
> > +                                           break;
> > +                                   default:
> > +                                           newdest[j++] = '%';
> > +                           }
> > +                   }
> > +                   if(tmp->destination[i] == '\\')
> > +                           i++;
> > +                   newdest[j] = tmp->destination[i];
> 
> The code here is... hairy. There is too much i++ and j++ for me. For 
> example, consider a tmp->destination = "%G%U" without any character 
> between. The "case 'G'" fills the group name, but the line right above 
> this comment copies '%' without expansion of '%U'.
> 
> In addition, with tmp->destination = "<something long>%G/%U" and really 
> long groupname exceeding FILENAME_MAX in newdest, the "case 'G'" fills 
> up the newdest and the aforementioned line above writes one character 
> *behind* the newdest. And expanding '%U' goes completely crazy, 
> FILENAME_MAX-j gets negative, converted to (unsigned) size_t, 
> overwriting who knows what.

I see, thanks for pointing that out, I overlooked it. Good sign to clean
it up a little bit. I guess that if I'll get something longer than
FILENAME_MAX I should truncate it anyway (correctly without writing to
some random memory).

Thanks for the review, will fix all mentioned problems and submit new
version.

-- 
        Michal Hrusecky <mic...@hrusecky.net>

------------------------------------------------------------------------------
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

Reply via email to