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. May be I should add PID check on the
server side too
> 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. But may be you are right in the point that this is a little bit
slow (pid to string and concatenate), I can replace it with
snprintf(path, FILENAME_MAX, "/proc/%s", argv[i]). Just concatenation
will be more faster.
>
>> +
>> + /* 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.
>
>> @@ -190,6 +195,6 @@ int main(int argc, char *argv[])
>> if (ret)
>> exit_code = 1;
>> }
>> - return exit_code;
>>
>> + return exit_code;
>> }
>
> Pointless hunk.
Agree)
>
------------------------------------------------------------------------------
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
