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

[Petr Bena]

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