Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--temp-basedir is broken with long usernames on OS X #888

Closed
The-Compiler opened this issue Aug 24, 2015 · 5 comments
Closed

--temp-basedir is broken with long usernames on OS X #888

The-Compiler opened this issue Aug 24, 2015 · 5 comments
Milestone

Comments

@The-Compiler
Copy link
Member

@acogneau reported he gets this with --temp-basedir:

qutebrowser.misc.ipc.ListenError: Error while listening to IPC server: QLocalServer::listen: Name error (error 2)

The reason is probably because there's a maximum char limit for the server name which means qutebrowser-alexandercogneau-4965f991c6f98de0b915c3989e8ca82c is too long.

Test script to get the max length:

from PyQt5.QtNetwork import QLocalServer
from PyQt5.QtWidgets import QApplication

app = QApplication([])

for i in range(1, 129):
    server = QLocalServer()
    name = 'x' * i
    ok = server.listen(name)
    if ok:
        print('{}: ok'.format(i))
    else:
        print('{}: error {} - {}'.format(i, server.serverError(),
                                         server.errorString()))

    print('  {}'.format(server.fullServerName()))

    server.close()
    server.deleteLater()
    QLocalServer.removeServer(name)

    if not ok:
        break

Should the username even be added with a non-default basedir?

@The-Compiler
Copy link
Member Author

For @acogneau, the maximum length which is okay is 54 chars, which uses the path /var/folders/yh/4lgdxxj16f7243s30d14rkr80000gn/T/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx (which is 104 chars).

@The-Compiler
Copy link
Member Author

This is going to get messy...

New path

Linux / OS X

We should probably switch to using a custom location on OS X and Linux, which should also be more secure (i.e. preventing other users on the same system from opening pages in your qutebrowser instance - which seems to be impossible now already, but better safe than sorry):

the easiest solution is to use QStandardDirs::RuntimeLocation for regular Unix and a private, user-only subdir of $TMPDIR on OS X, chmoded to the user

We can pass a full path to QLocalServer.listen.

The name then would just be qutebrowser with the default datadir, and qutebrowser-<md5> otherwise.

Windows

Since it uses some \\.\pipe\... path on Windows, we should still use the qutebrowser-<username>-<md5> there - the maximum length should be bigger there.

Migration

We still have to check the old location to make sure we don't corrupt an user's data when they upgrade but still have an old instance running...

What's unclear is how to find out if the given socket exists, ideally without waitForConnected. Maybe something like:

s = QLocalSocket()
s.connectToServer('foo')
assert s.error() == 0

works?

>>> s = QLocalSocket()
>>> s.setServerName('foo')
>>> s.fullServerName()
''
>>> s.connectToServer('foo')
>>> s.error()
2
>>> s.errorString()
'QLocalSocket::connectToServer: Invalid name'

If the check is expensive, I could probably also write something like old-ipc-name-checked = true to the state config, and if the old path is gone write that to the state - then I'd only have to check the first time.

@The-Compiler The-Compiler added this to the v0.4 milestone Sep 6, 2015
The-Compiler added a commit that referenced this issue Sep 6, 2015
@The-Compiler
Copy link
Member Author

I'm working on this, but there are still a few things missing:

  • Temporary directory should be used on OS X - it's user-, but not application-specific - so we probably can create a qutebrowser subdir in it, chmod 700 it, and have an ipc file/fifo in there.
  • "To ensure that your files are not removed, they should have their access time timestamp modified at least once every 6 hours of monotonic time or the 'sticky' bit should be set on the file." (OS X also seems to clean up things all 3 days)
  • The permissions should be adjusted
    • OS X: Make sure the FIFO is in a chmod 700'ed subdir
    • Linux: Already okay because we use the XDG runtime dir
    • Windows: QLocalServer.setSocketOptions(QLocalServer.UserAccessOption) should be called
  • Tests for all this stuff

The-Compiler added a commit that referenced this issue Sep 8, 2015
First trying the legacy path and then using the new one works fine now, but the
permissions are still wrong.
The-Compiler added a commit that referenced this issue Sep 9, 2015
First trying the legacy path and then using the new one works fine now, but the
permissions are still wrong.
The-Compiler added a commit that referenced this issue Sep 9, 2015
First trying the legacy path and then using the new one works fine now, but the
permissions are still wrong.
The-Compiler added a commit that referenced this issue Sep 9, 2015
First trying the legacy path and then using the new one works fine now, but the
permissions are still wrong.
The-Compiler added a commit that referenced this issue Sep 9, 2015
First trying the legacy path and then using the new one works fine now, but the
permissions are still wrong.
The-Compiler added a commit that referenced this issue Sep 9, 2015
First trying the legacy path and then using the new one works fine now, but the
permissions are still wrong.
@The-Compiler
Copy link
Member Author

This is now fixed except for setting the atime.

The-Compiler added a commit that referenced this issue Sep 10, 2015
@The-Compiler
Copy link
Member Author

After over a week of struggling, this should be fixed and working on every platform... Finally! 🎉

The-Compiler added a commit that referenced this issue Feb 22, 2019
With Qt 5.12, standarddir.runtime() gives us a path in /private/var/folders/...
instead of /var/folders/... like before. Due to that change, the path length is
105 chars, which is too long for a named socket (104 seems to be okay).

The complete name is just slightly too long, so using i- instead of ipc- fixes
things...

Fixes #4471
See #888
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant