process_singleton_posix: Remove the singleton lock file is not a symlink

process_singleton_posix: Remove the singleton lock file is not a symlink

The singleton lock file is meant to be used to check when another instance of
chromium is already running, so that a second instance being spawn can detect
that situation and send the needed data to the original browser before exiting.

For that reason, the singleton lock file is implemented as a symlink that encodes
the hostname and the PID for the original instance as the (non-existent) target,
which is information that will be checked on startup, automatically removing the
lock file if needed (e.g. original PID no longer valid, wrong hostname...).

However, one case that is not contemplated is whether the singleton lock file is
something else other than a symlink, which will be identied by ParseLockPath as a
"no lock present" situation without deleting the non-symlink file, and therefore
causing chromium to fail once it tries to create the lock later on during startup.

To prevent against this (unlikely, but possible) scenario, simply add some extra
check to ParseLockPath to make sure that only symlinks are allowed for the lock
file, and that anything else will be deleted before returning false ("no lock").

BUG=612453
R=eroman@chromium.org

[mario.endlessm, 2016-05-17 14:57:53]

Initial patch proposed, let me know what you think.

Thanks!

[Lei Zhang, 2016-05-17 17:53:17]

Description was changed from

==========
process_singleton_posix: Remove the singleton lock file is not a symlink

The singleton lock file is meant to be used to check when another instance of
chromium is already running, so that a second instance being spawn can detect
that situation and send the needed data to the original browser before exiting.

For that reason, the singleton lock file is implemented as a symlink that
encodes
the hostname and the PID for the original instance as the (non-existent) target,
which is information that will be checked on startup, automatically removing the
lock file if needed (e.g. original PID no longer valid, wrong hostname...).

However, one case that is not contemplated is whether the singleton lock file is
something else other than a symlink, which will be identied by ParseLockPath as
a
"no lock present" situation without deleting the non-symlink file, and therefore
causing chromium to fail once it tries to create the lock later on during
startup.

To prevent against this (unlikely, but possible) scenario, simply add some extra
check to ParseLockPath to make sure that only symlinks are allowed for the lock
file, and that anything else will be deleted before returning false ("no lock").

BUG=612453
R=eroman@chromium.org
==========

to

==========
process_singleton_posix: Remove the singleton lock file is not a symlink

The singleton lock file is meant to be used to check when another instance of
chromium is already running, so that a second instance being spawn can detect
that situation and send the needed data to the original browser before exiting.

For that reason, the singleton lock file is implemented as a symlink that
encodes
the hostname and the PID for the original instance as the (non-existent) target,
which is information that will be checked on startup, automatically removing the
lock file if needed (e.g. original PID no longer valid, wrong hostname...).

However, one case that is not contemplated is whether the singleton lock file is
something else other than a symlink, which will be identied by ParseLockPath as
a
"no lock present" situation without deleting the non-symlink file, and therefore
causing chromium to fail once it tries to create the lock later on during
startup.

To prevent against this (unlikely, but possible) scenario, simply add some extra
check to ParseLockPath to make sure that only symlinks are allowed for the lock
file, and that anything else will be deleted before returning false ("no lock").

BUG=612453
R=eroman@chromium.org
==========

[Lei Zhang, 2016-05-17 17:53:17]

thestig@chromium.org changed reviewers:
+ mattm@chromium.org

[Lei Zhang, 2016-05-17 17:53:40]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2016-05-17 17:53:41]

+mattm

[Lei Zhang, 2016-05-17 17:53:53]

thestig@chromium.org changed reviewers:
- thestig@chromium.org

[eroman, 2016-05-17 18:13:13]

This doesn't look right to me, but will defer to someone more knowledgeable
about the lock.

 * Why are we getting other files in place of the lock? Are you experiencing
this during normal uses, or are you the one creating the file?

 * Simply deleting the unexpected file when noticed also doesn't seem right. It
comes back to the question of what is this file and why is it there.

 * Suffers from TOCTTOU

I think a better solution would be to print more verbose logging about what went
wrong and let the user resolve it -- assuming the existence of this file is
truly unexpected.

If however there are cases where Chrome can get itself into this mess, then
maybe something of this manner could be justified.

[mattm, 2016-05-17 18:21:57]

On 2016/05/17 18:13:13, eroman wrote:
> This doesn't look right to me, but will defer to someone more knowledgeable
> about the lock.
> 
>  * Why are we getting other files in place of the lock? Are you experiencing
> this during normal uses, or are you the one creating the file?
> 
>  * Simply deleting the unexpected file when noticed also doesn't seem right.
It
> comes back to the question of what is this file and why is it there.
> 
>  * Suffers from TOCTTOU
> 
> I think a better solution would be to print more verbose logging about what
went
> wrong and let the user resolve it -- assuming the existence of this file is
> truly unexpected.
> 
> If however there are cases where Chrome can get itself into this mess, then
> maybe something of this manner could be justified.

I'm also feeling negative on this. Aside from the issues with the patch, there
are a billion ways you can break chrome by messing about in the config dir, we
obviously can't handle them all. Agree that we shouldn't try to handle this
unless it's something that can happen by itself.

Also the errors seem relatively helpful to me, here's the whole list I get when
I tried:
[17989:17989:0517/111714:ERROR:process_singleton_posix.cc(246)]
readlink(/tmp/chrm6/SingletonLock) failed: Invalid argument
[17989:17989:0517/111714:ERROR:process_singleton_posix.cc(246)]
readlink(/tmp/chrm6/SingletonLock) failed: Invalid argument
[17989:17989:0517/111714:ERROR:process_singleton_posix.cc(270)] Failed to create
/tmp/chrm6/SingletonLock: File exists
[17989:17989:0517/111714:ERROR:process_singleton_posix.cc(246)]
readlink(/tmp/chrm6/SingletonLock) failed: Invalid argument
[17989:17989:0517/111714:ERROR:chrome_browser_main.cc(1379)] Failed to create a
ProcessSingleton for your profile directory. This means that running multiple
instances would start multiple browser processes rather than opening a new
window in the existing process. Aborting now to avoid profile corruption.

[mario.endlessm, 2016-05-18 10:15:07]

Hi, thanks a lot for the quick reply, let me answer these points inline below...

On 2016/05/17 18:13:13, eroman wrote:
> This doesn't look right to me, but will defer to someone more knowledgeable
> about the lock.
> 
>  * Why are we getting other files in place of the lock? Are you experiencing
> this during normal uses, or are you the one creating the file?

It's hard to tell. We got a report from an user who somehow run into this state
which would prevent him to run chromium at all, and he was not knowledgeable
enough to debug it by running it from terminal, checking the output there (or
from the journal) and then figure out that he would have to delete the lock
manually.

But truth to be told, I have no idea on how he ended up in such a messed up
situation. In my case the only way I could reproduce it is by manually creating
a SingletonLock regular file (or directory) myself before launching chromium,
which is why I came up with this patch.

>  * Simply deleting the unexpected file when noticed also doesn't seem right.
It
> comes back to the question of what is this file and why is it there.

Well, my understanding from reading the code in process_singleton_posix.cc (and
the big explanation in the comment at the beginning of the file) is that this
file is indeed expected to be a symlink, and actually one that contains some
logic in the (non-existent) target, which should be of the form
<hostname>-<PID>.

By reading the code in ProcessSingleton::NotifyOtherProcessWithTimeout(), I can
see how some sanity checks are done to actually validate that the SingletonLock
symlink is indeed pointing to a valid target of that form, and that it already
decides to unlink it if it detects some invalid situation (e.g. invalid
hostname, invalid PID). What my patch does is simply adding an extra check that
is missing IMHO: that the SingletonLock symlink is indeed a symlink and not
something else, in which case it would delete it in a similar fashion to what's
already done if an invalid hostname or PID is found in the target of the
symlink.

So, it's not about "simply deleting the unexpected file when noticed", it's
about ensuring that the contract establish between this code and the file (which
is also explained in the comment at the beginning of this file) is respected by
adding this additional check, that IMHO should be there.
 
>  * Suffers from TOCTTOU

Hmmm.. good point. Perhaps a better place for this would be
ProcessSingleton::NotifyOtherProcessWithTimeout() then, together with the rest
of calls to UnlinkPath(lock_path_)?

> I think a better solution would be to print more verbose logging about what
went
> wrong and let the user resolve it -- assuming the existence of this file is
> truly unexpected.

That would be useful too, but only to experienced users that will go and check
the output from command line and/or the logs, which is not the case in our
particular scenario.

Still, worth adding that change to this patch set too, IMHO.

> If however there are cases where Chrome can get itself into this mess, then
> maybe something of this manner could be justified.

As I said, I know this happened for some reason to one of our users, even though
I can't explain why.

I understand this is not the best way to justify the patch, but in the other
hand I think it could be considered as in an additional check needed to make
sure that the SingletonLock file is properly validated, together with the other
checks that are already in place and that would unlink the file when needed.

What do you think?

[mario.endlessm, 2016-05-18 10:21:56]

On 2016/05/17 18:21:57, mattm wrote:
> [...]
> I'm also feeling negative on this. Aside from the issues with the patch, there
> are a billion ways you can break chrome by messing about in the config dir, we
> obviously can't handle them all. Agree that we shouldn't try to handle this
> unless it's something that can happen by itself.

As I mentioned in my previous comment, the patch is not about handling one of
the billion possible ways something could go wrong, but about checking and
ensuring that the described contract with this file is respected, running the
apropriate correcting action (delete the file) when we detect it's not the case,
as it's already done in other cases (invalid hostname, invalid PID).

Also, as I mentioned already, this issue happened by itself to one of our users
for some reason, which is what prompted this patch. How he got into that
situation is fairly a mistery to me :-), but still something that made me think
this additional check would be useful. 

> Also the errors seem relatively helpful to me, here's the whole list I get
when
> I tried:
> [...]

Yes, they are useful but only if you know how to check them :). Otherwise you're
left with a browser that refuses to run, and with little clue on why that
happens, or how to fix it.

Please let me know what you think after these comments. I'm more than happy to
iterate over this patch to make it better and more likely to be accepted
upstream.

Thanks!

[eroman, 2016-05-18 19:44:30]

Thanks Mario, I appreciate that you are trying to fix a problem for your user!

My suggestion for moving forward is to gather more data on how often and why
this happens, in order to better justify a solution.

If it is just a single user that encountered this once, for unknown reasons,
that probably isn't going to meet our bar. And in fact that would also mean it
won't be an ongoing issue for you to worry about ;)

If on the other hand this is something reproducible that is happening to other
users, please add the data to the bug report.

Cheers.

[mario.endlessm, 2016-05-19 09:27:32]

On 2016/05/18 19:44:30, eroman wrote:
> Thanks Mario, I appreciate that you are trying to fix a problem for your user!
> 
> My suggestion for moving forward is to gather more data on how often and why
> this happens, in order to better justify a solution.
> 
> If it is just a single user that encountered this once, for unknown reasons,
> that probably isn't going to meet our bar. And in fact that would also mean it
> won't be an ongoing issue for you to worry about ;)
> 
> If on the other hand this is something reproducible that is happening to other
> users, please add the data to the bug report.
> 
> Cheers.

Fair enough. I still think that a sanity check to make sure that
process_singleton_posix.cc meets the established contract would not hurt, but I
also see the point of not adding extra logic without proper justification, so
it's ok I gues.

I'll try to get a better understanding, if possible, of how on earth this
happened in the first place (since I can't really see how that could be either)
and post any update I might have here.

Thanks for taking your time to look into this in any case, much appreciated.