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

Socket buffer fix #1720

Merged

Conversation

varjolintu
Copy link
Member

Increases the maximum buffer size of keepassxc-protocol and Unix Sockets to 1M. This is the maximum messaging size for a single message with native messaging.

Motivation and context

Solves issues where the message size goes over the maximum default of Unix Socket buffer 8k bytes.

How has this been tested?

Manually.

Types of changes

  • ✅ Bug fix (non-breaking change which fixes an issue)

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

@@ -142,6 +142,14 @@ void NativeMessagingHost::newLocalConnection()
void NativeMessagingHost::newLocalMessage()
{
QLocalSocket* socket = qobject_cast<QLocalSocket*>(QObject::sender());
if (socket) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just put this code after the if (!socket....) check on line 154, no need for additional checks

@@ -18,10 +18,19 @@
#include <QCoreApplication>
#include "NativeMessagingHost.h"

enum { max_length = 1024*1024 };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this once defined in NativeMessagingBase.h

@@ -25,7 +25,7 @@
#include "gui/DatabaseTabWidget.h"
#include "core/Entry.h"

enum { max_length = 16*1024 };
enum { max_length = 1024*1024 };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should define this in NativeMessagingBase.h since it is shared between the proxy and browser code. Also, define as a constant: static const int NATIVE_MSG_MAX_LENGTH = 1024*1024; with all caps being the standard convention

@varjolintu varjolintu force-pushed the socket_buffer_fix branch 2 times, most recently from d666625 to e7ffd61 Compare April 1, 2018 08:35
@droidmonkey
Copy link
Member

We may want to add a test that passes a very large message.

@droidmonkey droidmonkey merged commit 3a92e4a into keepassxreboot:release/2.3.2 Apr 3, 2018
@varjolintu varjolintu deleted the socket_buffer_fix branch April 3, 2018 05:14
@emeygret
Copy link

Hi all, this patch break windows compilation :
``C:/msys64/home/emeygret/keepassxc/src/proxy/NativeMessagingHost.cpp: In constructor 'NativeMessagingHost::NativeMessagingHost()':
C:/msys64/home/emeygret/keepassxc/src/proxy/NativeMessagingHost.cpp:30:32: error: 'SOL_SOCKET' was not declared in this scope
setsockopt(socketDesc, SOL_SOCKET, SO_SNDBUF, &max, sizeof(max));
^~~~~~~~~~
C:/msys64/home/emeygret/keepassxc/src/proxy/NativeMessagingHost.cpp:30:44: error: 'SO_SNDBUF' was not declared in this scope
setsockopt(socketDesc, SOL_SOCKET, SO_SNDBUF, &max, sizeof(max));
^~~~~~~~~
C:/msys64/home/emeygret/keepassxc/src/proxy/NativeMessagingHost.cpp:30:9: error: 'setsockopt' was not declared in this scope
setsockopt(socketDesc, SOL_SOCKET, SO_SNDBUF, &max, sizeof(max));
^~~~~~~~~~
C:/msys64/home/emeygret/keepassxc/src/proxy/NativeMessagingHost.cpp:30:9: note: suggested alternative: 'getopt'
setsockopt(socketDesc, SOL_SOCKET, SO_SNDBUF, &max, sizeof(max));
^~~~~~~~~~
getopt
make[2]: *** [src/proxy/CMakeFiles/proxy.dir/build.make:89: src/proxy/CMakeFiles/proxy.dir/NativeMessagingHost.cpp.obj] Error 1
make[1]: *** [CMakeFiles/Makefile2:735: src/proxy/CMakeFiles/proxy.dir/all] Error 2
make[1]: *** Attente des tâches non terminées....
C:/msys64/home/emeygret/keepassxc/src/browser/NativeMessagingHost.cpp: In member function 'void NativeMessagingHost::newLocalMessage()':
C:/msys64/home/emeygret/keepassxc/src/browser/NativeMessagingHost.cpp:153:32: error: 'SOL_SOCKET' was not declared in this scope
setsockopt(socketDesc, SOL_SOCKET, SO_SNDBUF, &max, sizeof(max));
^~~~~~~~~~
C:/msys64/home/emeygret/keepassxc/src/browser/NativeMessagingHost.cpp:153:32: note: suggested alternative: 'QSSLSOCKET_H'
setsockopt(socketDesc, SOL_SOCKET, SO_SNDBUF, &max, sizeof(max));
^~~~~~~~~~
QSSLSOCKET_H
C:/msys64/home/emeygret/keepassxc/src/browser/NativeMessagingHost.cpp:153:44: error: 'SO_SNDBUF' was not declared in this scope
setsockopt(socketDesc, SOL_SOCKET, SO_SNDBUF, &max, sizeof(max));
^~~~~~~~~
C:/msys64/home/emeygret/keepassxc/src/browser/NativeMessagingHost.cpp:153:9: error: 'setsockopt' was not declared in this scope
setsockopt(socketDesc, SOL_SOCKET, SO_SNDBUF, &max, sizeof(max));
^~~~~~~~~~
C:/msys64/home/emeygret/keepassxc/src/browser/NativeMessagingHost.cpp:153:9: note: suggested alternative: 'getopt'
setsockopt(socketDesc, SOL_SOCKET, SO_SNDBUF, &max, sizeof(max));
^~~~~~~~~~
getopt

droidmonkey added a commit that referenced this pull request May 8, 2018
- Enable high entropy ASLR on Windows [#1747]
- Enhance favicon fetching [#1786]
- Fix crash on Windows due to autotype [#1691]
- Fix dark tray icon changing all icons [#1680]
- Fix --pw-stdin not using getPassword function [#1686]
- Fix placeholders being resolved in notes [#1907]
- Enable auto-type start delay to be configurable [#1908]
- Browser: Fix native messaging reply size [#1719]
- Browser: Increase maximum buffer size [#1720]
- Browser: Enhance usability and functionality [#1810, #1822, #1830, #1884, #1906]
- SSH Agent: Parse aes-256-cbc/ctr keys [#1682]
- SSH Agent: Enhance usability and functionality [#1677, #1679, #1681, #1787]
jtl999 pushed a commit to jtl999/keepassxc that referenced this pull request Jul 29, 2018
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

Successfully merging this pull request may close these issues.

3 participants