KDSingleApplication: Do not exceed maximum socket name length #45
Conversation
jonaski
force-pushed
the
socketname_length
branch
3 times, most recently
from
6a7b309 to
62df868
Compare
|
The macOS CI is broken because |
|
#46 fixes the macOS build |
|
any chance to get a minimal repro committed as a test ? (something that fails without this patch) |
In cases where there is a long name given (usually the application name) or a long username, `QLocalServer::listen` fails because `sockaddr_un.sun_path` has a char array of 108 on Linux, and 104 on BSD/macOS. On macOS, the TMPDIR path is long, ie.: `/var/folders/zg/b242mhvd125344h5zrnpkzk80000gn/T/`, so 49 characters just for the temp path alone. To fix this, use the uid instead of the username if the total length of the socket name is expected to exceed the maximum length. If it still exceeds the maximum allowed length, ie.: the given name is too long, chop the socket name which cuts off the last part of the name. I've tested this to work on both Linux and macOS, Windows should be unaffected by these changes. Fixes strawberrymusicplayer/strawberry#1603
for more information, see https://pre-commit.ci
jonaski
force-pushed
the
socketname_length
branch
from
62df868 to
6bb18d0
Compare
|
@iamsergio I added a test now |
|
thanks! seems to be failing though ? on windows Qt 5 |
Fixed now |
|
Ping? |
|
Thanks! |
| @@ -30,6 +30,9 @@ | |||
| #include <sys/types.h> | |||
| #include <unistd.h> | |||
| #include <pwd.h> | |||
| #if defined(Q_OS_LINUX) | |||
There was a problem hiding this comment.
Can this be made generic for Unix, rather than Linux-specific? How about #include <sys/un.h> (man 7 unix I guess).
| constexpr int maxSocketNameLength = UNIX_PATH_MAX - 1; | ||
| #else | ||
| constexpr int maxSocketNameLength = 103; // BSD and macOS | ||
| #endif |
There was a problem hiding this comment.
This then could become sizeof(sockaddr_un::sun_path)
| m_socketName += sessionId; | ||
| } | ||
| sessionId = qEnvironmentVariable("XDG_SESSION_ID"); | ||
| } |
There was a problem hiding this comment.
(style) AFAICS, no braces on single line "bodies" (if/for/while/...) is used around the rest of the project...
| m_socketName += QStringLiteral("-"); | ||
| m_socketName += name; | ||
|
|
||
| int fullSocketNameLength = tempPathLength + m_socketName.length(); | ||
| #if defined(Q_OS_LINUX) || defined(Q_OS_QNX) | ||
| fullSocketNameLength += 1; // PlatformSupportsAbstractNamespace, see qlocalserver_unix.cpp |
There was a problem hiding this comment.
Are we using the abstract namespace?
| qCDebug(kdsaLocalSocket) << "Chopping socket name because it is longer than" << maxSocketNameLength; | ||
| m_socketName.chop(fullSocketNameLength - maxSocketNameLength); | ||
| } | ||
|
|
There was a problem hiding this comment.
Ruminations.
- Is it better to chop and possibly getting a clash (multiple independent applications may accidentally use the same socket), or fail to listen?
- Although, if we fail to listen, it's unclear what the user could do to address the problem.
- We're doing a lot of work in here, but this "determining the max length" should really be exposed as an API on QLocalServer/QLocalSocket...
In cases where there is a long name given (usually the application name) or a long username,
QLocalServer::listenfails becausesockaddr_un.sun_pathhas a char array of 108 on Linux, and 104 on BSD/macOS. On macOS, the TMPDIR path is long, ie.:/var/folders/zg/b242mhvd125344h5zrnpkzk80000gn/T/, so 49 characters just for the temp path alone. To fix this, use the uid instead of the username if the total length of the socket name is expected to exceed the maximum length. If it still exceeds the maximum allowed length, ie.: the given name is too long, chop the socket name which cuts off the last part of the name.I've tested this to work on both Linux and macOS, Windows should be unaffected by these changes.
Fixes strawberrymusicplayer/strawberry#1603