On Thu 14-07-11 07:55:12, Nikiforov Alex wrote:
> On 07/12/2011 07:16 PM, Michal Hocko wrote:
> > On Fri 08-07-11 13:58:25, Nikiforov Alex wrote:
> >> From 4ff9e938522d1f5980706cbbb09019493ea7425a Mon Sep 17 00:00:00 2001
> >> From: Alex Nikiforov<a.nikifo...@samsung.com>
> >> Date: Fri, 8 Jul 2011 13:09:57 +0400
> >>
> >> Move PID check code from the server to the client. We dont need any
> >> read()/write() if it's not valid.
> >
> > What happens if somebody else use the socket and inject an invalid pid?
> > Can this be misused - e.g. DOS attack?
> >
> I think about it before I made my patch. I think daemon should work only
> with libcg tools and if you need work with daemon you should work
> through for example cgclassify.
How do you want to force that behavior? Well you can restrict access by
-u resp. -g parameters to the daemon should be defensive anyway.
> May be I should add PID check on the server side too
Yes, use vector read and then check the pid.
> > AFAICS, if we didn't check the validity we would simply call
> > cgre_store_unchanged_process and increase internally used storage which
> > can grow without any bounds. Existence of a pid limits us reasonably.
> >
> >>
> >> Signed-off-by: Alex Nikiforov<a.nikifo...@samsung.com>
> >>
> >> ---
> >> src/daemon/cgrulesengd.c | 7 -------
> >> src/tools/cgclassify.c | 11 ++++++++---
> >> 2 files changed, 8 insertions(+), 10 deletions(-)
> >>
> > [...]
> >> diff --git a/src/tools/cgclassify.c b/src/tools/cgclassify.c
> >> index 397b725..029d091 100644
> >> --- a/src/tools/cgclassify.c
> >> +++ b/src/tools/cgclassify.c
> > [...]
> >> @@ -167,8 +168,12 @@ int main(int argc, char *argv[])
> >> }
> >>
> >> for (i = optind; i< argc; i++) {
> >> +
> >> pid = (uid_t) strtol(argv[i],&endptr, 10);
> >> - if (endptr[0] != '\0') {
> >> + snprintf(path, FILENAME_MAX, "/proc/%d", pid);
> >
> > You are doing this string operation even though you get an invalid
> > argument. I know that error handling gets more complicated but I do not
> > see any reason to to do snprintf if you get a mess.
> I think this is unreasonable to write if() { if () } here bcoz it's
> ugly.
Heh, you can do it nicely as well ;)
> But may be you are right in the point that this is a little bit
> slow (pid to string and concatenate),
It is not about slowdown because it would be hardly noticable. It is
more about - do not execute pointless code.
> I can replace it with
> snprintf(path, FILENAME_MAX, "/proc/%s", argv[i]). Just concatenation
> will be more faster.
But you need the pid number... Doing a conversion and fail on non-number
is more effective than doing a syscall and then do a conversion for the
fail path.
> >
> >> +
> >> + /* check PID */
> >> + if (endptr[0] != '\0' || stat(path,&buff_stat)) {
> >> /* the input argument was not a number */
> >> fprintf(stderr, "Error: %s is not valid pid.\n",
> >> argv[i]);
> >
> > The check should be done for sure.
> Dont quite understand you. We check PID for number and if it's true call
> stat() (all logical constructions check from right to left) else stat()
> wont call.
Sorry that I wasn't clear enough. I just agree that the client should
test the given pid as well because you do not want let clients to
pointlessly trigger daemon and potentially overload it.
--
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9
Czech Republic
------------------------------------------------------------------------------
AppSumo Presents a FREE Video for the SourceForge Community by Eric
Ries, the creator of the Lean Startup Methodology on "Lean Startup
Secrets Revealed." This video shows you how to validate your ideas,
optimize your ideas and identify your business strategy.
http://p.sf.net/sfu/appsumosfdev2dev
_______________________________________________
Libcg-devel mailing list
Libcg-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libcg-devel
