[Labs-l] [tools] New version of take

[Petr Bena]

You say: you are still vulnerable - where? to what?

On Sun, Jun 23, 2013 at 9:04 AM, Petr Bena <benapetr at gmail.com> wrote:
> Purpose of the struct is that if I forgot to close some FD it would
> get automagically closed once the object is destroyed (it's created on
> stack, so once it get removed from stack it get auto-closed)
>
> your change is correct, I should have make sure that closing it
> multiple times doesn't cause troubles, however I don't see where I am
> closing it three times?
>
> On Sun, Jun 23, 2013 at 12:25 AM, Platonides <platonides at gmail.com> wrote:
>> On 22/06/13 20:24, Petr Bena wrote:
>>>
>>> resolved.
>>
>>
>> No, you're still vulnerable (1609de5f).
>> Moreover, your are changing a file different than the one you checked.
>>
>> I also noticed that you are closing each descriptor thrice.
>>
>> One instance is trivially fixed by:
>>>
>>> diff --git a/include/Take.h b/include/Take.h
>>> index dd063f3..4fa2a41 100644
>>> --- a/include/Take.h
>>> +++ b/include/Take.h
>>> @@ -20,7 +20,7 @@ class Take
>>>          int fd;
>>>          FD(int f): fd(f)                { };
>>>          ~FD()                           { Close(); };
>>> -        void Close()                    { if(fd >= 0) close(fd); };
>>> +        void Close()                    { if(fd >= 0) close(fd); fd = -1;
>>> };
>>>          operator int (void) const       { return fd; };
>>>      };
>>
>>
>>
>> The other close was hidden in copied objects (why did you need an object?
>> This would have been much easier simply passing the fds!)
>>
>>> diff --git a/include/Take.h b/include/Take.h
>>> index dd063f3..ac8a417 100644
>>> --- a/include/Take.h
>>> +++ b/include/Take.h
>>> @@ -29,10 +29,10 @@ class Take
>>>          virtual ~Take();
>>>      protected:
>>>      private:
>>> -        bool Overtake(string path, FD fd);
>>> +        bool Overtake(string path, FD& fd);
>>>          bool Verify(string path);
>>>          static int Callback(const char* path, const struct stat *sb, int
>>> typeflag, struct FTW *ftwbuf);
>>> -        static void ChangeOwner(string path, uid_t owner, FD fd, bool
>>> ChangeGroup);
>>> +        static void ChangeOwner(string path, uid_t owner, FD& fd, bool
>>> ChangeGroup);
>>>          static bool CheckGroups(struct stat info);
>>>          static bool CheckHL(string path);
>>>          static bool CheckHL(struct stat info);
>>> diff --git a/src/Take.cpp b/src/Take.cpp
>>> index 8a111d6..accfc02 100644
>>> --- a/src/Take.cpp
>>> +++ b/src/Take.cpp
>>> @@ -126,7 +126,7 @@ int Take::Callback(const char* path, const struct stat
>>> *sb, int typeflag, struct
>>>      return 0;
>>>  }
>>>
>>> -void Take::ChangeOwner(string path, uid_t owner, Take::FD fd, bool
>>> ChangeGroup)
>>> +void Take::ChangeOwner(string path, uid_t owner, Take::FD& fd, bool
>>> ChangeGroup)
>>>  {
>>>      if (!ChangeGroup)
>>>      {
>>> @@ -215,7 +215,7 @@ bool Take::CheckHL(string path)
>>>      return false;
>>>  }
>>>
>>> -bool Take::Overtake(string path, Take::FD fd)
>>> +bool Take::Overtake(string path, Take::FD& fd)
>>>  {
>>>      char * p = realpath(path.c_str(), NULL);
>>>
>>
>>
>> _______________________________________________
>> Labs-l mailing list
>> Labs-l at lists.wikimedia.org
>> https://lists.wikimedia.org/mailman/listinfo/labs-l