On 07/14/2011 12:18 PM, Michal Hocko wrote:
> 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.
>
fifo with proper rights
>> 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.
>
oK a PID is "valid", it's a digit and in right range, but it's not
exists. I think it's reasonable here - a first make fast check and then
make deep check and the code is quite clear.
>> 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.
>
at first I check for conversion result and second (if the PID is real
digit) check more deeply with stat().
>>>
>>>> +
>>>> + /* 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.
>
------------------------------------------------------------------------------
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
