[Development] [QLockFile] Whether it is reasonable to use 0644 permission ?

[Denis Shienkov]

Hi all.

As I can see, the QLockFile opens a lock-file with the 0644 permissions
in Linux, e.g. see QLockFile::LockError QLockFilePrivate::tryLock_sys() [1].

That means that only one user (owner) can modify this file. But it leads
to problems, for example, when this use-case:

1. User 'A' creates the lock-file for the some resource

2. The application crashed, and this lock-file stays undeleted/unchanged

3. User 'B' tries to use same free resource, but it is failed due to
QLockFile can not write appropriate pid to the lock-file, since user 'B'
has not access to modify this lock-file.

This happens, e.g. when user 'A' is root and so on.

Currently this problem belongs to QSerialPort which internally uses
QLockFile to indication of employment of the /dev/ttyXYZ resources.
When it is impossible to open serial port when its lock-file has been
created from other user. There are only one workaround - it is remove
this lock-file manually.

Why just do not use 0666 permissions for this?

BR,
Denis







[1]
http://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/io/qlockfile_unix.cpp#n181

[Thiago Macieira]

On quarta-feira, 27 de julho de 2016 09:49:14 PDT Denis Shienkov wrote:
> Why just do not use 0666 permissions for this?

We should use 0666 and let the umask determine which bits to exclude.

--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center

[Denis Shienkov]

> and let the umask determine which bits to exclude.

what do you mean here? why we need to use umask, and why we need to
exclude bits?


27.07.2016 10:01, Thiago Macieira пишет:
> On quarta-feira, 27 de julho de 2016 09:49:14 PDT Denis Shienkov wrote:
>> Why just do not use 0666 permissions for this?
> We should use 0666 and let the umask determine which bits to exclude.
>

[Albert Astals Cid]

FWIW this is slightly related to https://bugreports.qt.io/browse/QTBUG-53570

On Wed, Jul 27, 2016 at 9:31 AM, Denis Shienkov
<***@gmail.com> wrote:
>> and let the umask determine which bits to exclude.
>
> what do you mean here? why we need to use umask, and why we need to exclude
> bits?
>
>
> 27.07.2016 10:01, Thiago Macieira пишет:
>>
>> On quarta-feira, 27 de julho de 2016 09:49:14 PDT Denis Shienkov wrote:
>>>
>>> Why just do not use 0666 permissions for this?
>>
>> We should use 0666 and let the umask determine which bits to exclude.
>>
>
> _______________________________________________
> Development mailing list
> ***@qt-project.org
> http://lists.qt-project.org/mailman/listinfo/development

[Thiago Macieira]

On quarta-feira, 27 de julho de 2016 10:31:35 PDT Denis Shienkov wrote:
> > and let the umask determine which bits to exclude.
>
> what do you mean here? why we need to use umask, and why we need to
> exclude bits?

Because we don't have a choice. The umask bits apply when you create a fie.

--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center

[Oswald Buddenhagen]

On Wed, Jul 27, 2016 at 09:49:14AM +0300, Denis Shienkov wrote:
> Currently this problem belongs to QSerialPort which internally uses
> QLockFile to indication of employment of the /dev/ttyXYZ resources.
>
well, that's kinda the problem. this approach is a hack to work around
inadequate locking on the device itself. is this still a problem on
modern unixes?

> Why just do not use 0666 permissions for this?
>
because any malicious user can then sabotage other users' lock files.

also, isn't this actually mostly about the permissions of the directory
containing the locks? lock grabbing should be mostly about deleting and
re-creating the file.

[Thiago Macieira]

On quarta-feira, 27 de julho de 2016 11:37:51 PDT Oswald Buddenhagen wrote:
> also, isn't this actually mostly about the permissions of the directory
> containing the locks? lock grabbing should be mostly about deleting and
> re-creating the file.

That's also a good point. If you have permission to delete the stale lock,
problem solved.

If you don't have permission to delete it, then you're going to have a problem
to drop your own lock if you steal the lock by writing to the file.

Either way, the mode in the call should be 0666.

--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center

[Kevin Kofler]

Thiago Macieira wrote:
> If you don't have permission to delete it, then you're going to have a
> problem to drop your own lock if you steal the lock by writing to the
> file.

So this means that the permission should be 0644 to prevent that from
happening. Thus, I don't understand how you get to:

> Either way, the mode in the call should be 0666.

Kevin Kofler

[Thiago Macieira]

On quarta-feira, 27 de julho de 2016 22:15:45 PDT Kevin Kofler wrote:
> Thiago Macieira wrote:
> > If you don't have permission to delete it, then you're going to have a
> > problem to drop your own lock if you steal the lock by writing to the
> > file.
>
> So this means that the permission should be 0644 to prevent that from
> happening. Thus, I don't understand how you get to:

The permission of the file is irrelevant. The permission of the dir is the one
that matters.

> > Either way, the mode in the call should be 0666.

Because that will result in 0644 or 0664, depending on the user's umask. The
correct modes for creat() and open() should be only 0600, 0700, 0666 and 0777.
Anything else is suspicious and probably wrong. Like this case.

--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center

[Denis Shienkov]

So, as I understand, you agree that QLockFile should be fixed.. Who will
produce a patch? Thiago, maybe you can do it? :)

PS: Because I do not understand an idea with the umask... I simply would
replace 0644 with 0666 in sources... and nothing more..

28.07.2016 0:23, Thiago Macieira пишет:
> On quarta-feira, 27 de julho de 2016 22:15:45 PDT Kevin Kofler wrote:
>> Thiago Macieira wrote:
>>> If you don't have permission to delete it, then you're going to have a
>>> problem to drop your own lock if you steal the lock by writing to the
>>> file.
>> So this means that the permission should be 0644 to prevent that from
>> happening. Thus, I don't understand how you get to:
> The permission of the file is irrelevant. The permission of the dir is the one
> that matters.
>
>>> Either way, the mode in the call should be 0666.
> Because that will result in 0644 or 0664, depending on the user's umask. The
> correct modes for creat() and open() should be only 0600, 0700, 0666 and 0777.
> Anything else is suspicious and probably wrong. Like this case.
>

[Simon Hausmann]

Here you go :)


https://codereview.qt-project.org/166278



Simon

________________________________
From: Development <development-bounces+simon.hausmann=***@qt-project.org> on behalf of Denis Shienkov <***@gmail.com>
Sent: Thursday, July 28, 2016 8:34:03 AM
To: ***@qt-project.org
Subject: Re: [Development] [QLockFile] Whether it is reasonable to use 0644 permission ?

So, as I understand, you agree that QLockFile should be fixed.. Who will
produce a patch? Thiago, maybe you can do it? :)

PS: Because I do not understand an idea with the umask... I simply would
replace 0644 with 0666 in sources... and nothing more..

28.07.2016 0:23, Thiago Macieira ÐÉÛÅÔ:
> On quarta-feira, 27 de julho de 2016 22:15:45 PDT Kevin Kofler wrote:
>> Thiago Macieira wrote:
>>> If you don't have permission to delete it, then you're going to have a
>>> problem to drop your own lock if you steal the lock by writing to the
>>> file.
>> So this means that the permission should be 0644 to prevent that from
>> happening. Thus, I don't understand how you get to:
> The permission of the file is irrelevant. The permission of the dir is the one
> that matters.
>
>>> Either way, the mode in the call should be 0666.
> Because that will result in 0644 or 0664, depending on the user's umask. The
> correct modes for creat() and open() should be only 0600, 0700, 0666 and 0777.
> Anything else is suspicious and probably wrong. Like this case.
>

[Denis Shienkov]

>> Currently this problem belongs to QSerialPort which internally uses
QLockFile to indication of employment of the /dev/ttyXYZ resources.> is this still a problem on modern unixes?

> is this still a problem on modern unixes?

Yes, of course, it still is a problem...

There are one TIOCEXCL flag to make 'exclusive' access, but this flag
does not work if the root
user want to open an already 'locked' device, because the root ignores
TIOCEXCL..

So, stays one adequate way, it is use the lock files, where even root
won't be able to open an
already locked device (of course, if root's software parses lock-files,
and if looks for these lock-files in necessary directory).. :)

PS: So, Unix in this case is a ZOO of different solutions, which do not
work, IMHO...

[Oswald Buddenhagen]

On Wed, Jul 27, 2016 at 12:01:20AM -0700, Thiago Macieira wrote:
> On quarta-feira, 27 de julho de 2016 09:49:14 PDT Denis Shienkov wrote:
> > Why just do not use 0666 permissions for this?
>
> We should use 0666 and let the umask determine which bits to exclude.
>
how exactly is this response supposed to be helpful in any way?

On Wed, Jul 27, 2016 at 09:41:34AM +0200, Albert Astals Cid wrote:
> FWIW this is slightly related to https://bugreports.qt.io/browse/QTBUG-53570
>
it's probably the correct solution *if* qlockfile can be actually used
for this purpose at all without causing other problems.