Created attachment 249113 [details] v0 (completely untested, apply via `git am`)
Hi, I was also working on this update, but my work stalled. I noticed two important changeschanges: - minizip is now always required (at present it's registered as dependency only if the KEESHARE option is on), this one is easily fixed. - Project cmake tries to find libusb and fails The latter is blocking my work on the update at present, did not have time to investigate further.
(In reply to Guido Falsi from comment #1) Thanks for the info! Looking @ upstream repo and changelog, I found pull request [1], which contains CMakeLists.txt change: if(UNIX AND NOT APPLE) find_library(LIBUSB_LIBRARIES NAMES usb-1.0 REQUIRED) find_path(LIBUSB_INCLUDE_DIR NAMES libusb.h PATH_SUFFIXES "libusb-1.0" "libusb" REQUIRED) include_directories(SYSTEM ${LIBUSB_INCLUDE_DIR}) endif() Perhaps excluding FreeBSD there will help, but I didn't test it yet. [1]: https://github.com/keepassxreboot/keepassxc/pull/10092
(In reply to Anton Saietskii from comment #2) Ah, ok, so libusb is only required when YUBIKEY=on, and it can't work without it now. For me, personally, it's not an issue -- I don't use YubiKeys, but the port with its support enabled will be broken at the moment, which is I believe not something we want.
Created attachment 249119 [details] v1 (apply via `git am`) Ah, ok, so we got libusb in base system (without "-1.0" suffix). However, keepassxc still doesn't build, now failing while trying to use libusb header: /usr/local/libexec/ccache/c++ -DKEEPASSX_BUILDING_CORE -DQT_CONCURRENT_LIB -DQT_CORE_LIB -DQT_DBUS_LIB -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_NO_CAST_TO_ASCII -DQT_NO_DEBUG -DQT_NO_DEBUG_OUTPUT -DQT_NO_DEPRECATED_WARNINGS -DQT_NO_EXCEPTIONS -DQT_STRICT_ITERATORS -DQT_SVG_LIB -DQT_WIDGETS_LIB -DQT_X11EXTRAS_LIB -DWITH_APP_BUNDLE -I/wrkdirs/usr/ports/security/keepassxc/work/.build/src/keepassx_core_autogen/include -I/wrkdirs/usr/ports/security/keepassxc/work/keepassxc-2.7.7/src -I/wrkdirs/usr/ports/security/keepassxc/work/.build/src -I/wrkdirs/usr/ports/security/keepassxc/work/keepassxc-2.7.7/src/zxcvbn -I/usr/local/include/qt5/QtGui/5.15.12 -I/usr/local/include/qt5/QtGui/5.15.12/QtGui -I/usr/local/include/qt5/QtCore/5.15.12 -I/usr/local/include/qt5/QtCore/5.15.12/QtCore -isystem /usr/local/include/botan-2 -isystem /usr/local/include -isystem /usr/local/include/PCSC -isystem /usr/local/include/qt5 -isystem /usr/local/include/qt5/QtCore -isystem /usr/local/lib/qt5/mkspecs/freebsd-clang -isystem /usr/local/include/qt5/QtWidgets -isystem /usr/local/include/qt5/QtGui -isystem /usr/local/include/qt5/QtSvg -isystem /usr/local/include/qt5/QtConcurrent -isystem /usr/local/include/qt5/QtNetwork -isystem /usr/local/include/qt5/QtDBus -isystem /usr/local/include/qt5/QtX11Extras -O3 -pipe -march=skylake -fstack-protector-strong -fno-strict-aliasing -fno-common -fopenmp=libomp -Wall -Wextra -Wundef -Wpointer-arith -Wno-long-long -Wformat=2 -Wmissing-format-attribute -fvisibility=hidden -fvisibility-inlines-hidden -fstack-protector-strong -Wnon-virtual-dtor -Wold-style-cast -Woverloaded-virtual -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Werror=format-security -Wcast-align -Qunused-arguments -fsized-deallocation -Wno-deprecated-declarations -O3 -pipe -march=skylake -fstack-protector-strong -fno-strict-aliasing -DNDEBUG -std=gnu++17 -fPIC -fPIC -MD -MT src/CMakeFiles/keepassx_core.dir/gui/osutils/nixutils/DeviceListenerLibUsb.cpp.o -MF src/CMakeFiles/keepassx_core.dir/gui/osutils/nixutils/DeviceListenerLibUsb.cpp.o.d -o src/CMakeFiles/keepassx_core.dir/gui/osutils/nixutils/DeviceListenerLibUsb.cpp.o -c /wrkdirs/usr/ports/security/keepassxc/work/keepassxc-2.7.7/src/gui/osutils/nixutils/DeviceListenerLibUsb.cpp ../keepassxc-2.7.7/src/gui/osutils/nixutils/DeviceListenerLibUsb.cpp:71:21: error: no matching function for call to 'libusb_hotplug_register_callback' 71 | const int ret = libusb_hotplug_register_callback( | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/include/libusb.h:597:5: note: candidate function not viable: no known conversion from 'int *' to 'libusb_hotplug_callback_handle *' (aka 'libusb_hotplug_callback_handle_struct **') for 9th argument 597 | int libusb_hotplug_register_callback(libusb_context *ctx, libusb_hotplug_event events, libusb_hotplug_flag flags, int vendor_id, int product_id, int dev_class, libusb_hotplug_callback_fn cb_fn, void *user_data, libusb_hotplug_callback_handle *handle); | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../keepassxc-2.7.7/src/gui/osutils/nixutils/DeviceListenerLibUsb.cpp:110:5: error: no matching function for call to 'libusb_hotplug_deregister_callback' 110 | libusb_hotplug_deregister_callback(static_cast<libusb_context*>(m_ctx), handle); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/include/libusb.h:598:6: note: candidate function not viable: no known conversion from 'int' to 'libusb_hotplug_callback_handle' (aka 'libusb_hotplug_callback_handle_struct *') for 2nd argument 598 | void libusb_hotplug_deregister_callback(libusb_context *ctx, libusb_hotplug_callback_handle handle); | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2 errors generated. ninja: build stopped: subcommand failed. *** Error code 1
Digging into libusb code, I found that libusb_hotplug_callback_handle is significantly different -- it's simple int in original (upstream?) libusb while it's pointer to libusb_hotplug_callback_handle_struct in our base system implementation. I don't see (though may be wrong) any obvious or easy enough way to make it work, thus gonna close this PR soon if there will be no objections.
Created attachment 250521 [details] Update patch with libusb "fixes" Hi, I took some time to create a patch that allows it to compile. I'm using it on my machine and it works, but I don't have any USB security keys, so I can't test the libusb patches I've added. While it compiles and I think it should work, I'm not completely sure the pointer "gymnastics" I'm doing is completely correct and maybe the code I've written will utterly crash if actually used. Could someone test it with some USB device and see if it really works? Thanks in advance!
Reopening PR. BTW My patch also updates to the latest 2.7.8.
(In reply to Guido Falsi from comment #6) Thanks very much! I do have a yubikey so I can test, but probably later this week.
(In reply to Li-Wen Hsu from comment #8) Great! If it works I will also find time to send a PR upstream with the patches. I wrapped everything in ifdefs, so it should be fine being included upstream.
(In reply to Guido Falsi from comment #9) It's weird that I tested it on three machines, 14.0-R and -CURRENT, it all stuck in different places, like open key db or closing it, or shutting down the application. I'm using it with KDE5, although I don't think it would be an issue. I'll try with other WM later.
(In reply to Li-Wen Hsu from comment #10) Still doesn't work on XFCE :(
(In reply to Li-Wen Hsu from comment #11) As I said I'm not completely sure what I'm doing in my patches with pointers is correct. I'm not a C++ expert, I have only some theoretical knowledge about it and very little actual coding experience with it. Can you explain what is not working? is it crashing when you try to access the youbikey? Would you be able to get a backtrace from a DEBUG enabled binary? that could help understand where the issue is.
(In reply to Guido Falsi from comment #12) Actually, I'm not suprised. Not a C* expert also (in fact, not a programmer at all), but I think you can't just cast structure with a lot of fields to simple number. I mean, technically it's possible in C*, but I wouldn't expect it will work. It looks like relevant code from KeePassXC side should be significantly rewritten. Just for reference struct itself: struct libusb_hotplug_callback_handle_struct { TAILQ_ENTRY(libusb_hotplug_callback_handle_struct) entry; int events; int vendor; int product; int devclass; libusb_hotplug_callback_fn fn; void *user_data; }; Doesn't look for me like something cat be treated as just a single int...
(In reply to Anton Saietskii from comment #13) I'd agree, but what I'm actually juggling around are pointers to a struct (and a pointer to a pointer at one point, because that's what the function argument asks for). I actually never cast to an int (if I do that's a mistake) although casting a pointer to int and then back should not mangle it, as long as int and void* have the same length (should be the case for most modern architectures). Not good practice but should work most of the time. Anyway that's not what I'm doing. The upstream code uses a libUSB that gives back handles in form of integers (I guess they are indexes to some internal array). FreeBSD libUSB handles back handles that are actually pointers to a struct. I tried to modify the code to use those pointers in place of the ints. In theory it should work, but maybe I missed something somewhere. I'll go back to the code. The only part that gives me some difficulty is the m_listeners thing, I don't know exactly hot that works.
(In reply to Guido Falsi from comment #14) I made a mistake in my previous response. While on amd64 the word size is 64 bit and pointers are 64 bit long, int is not strictly defined, any compiler can define that, and it happens to be 32 bits (for compatibility reasons). So I was wrong, casting a pointer to an int will most probably mangle it. OTOH casting a pointer to any other 64 bit type (in theory long long...) should not mangle it. But this is not relevant in this case since I'm not intentionally casting pointers to other types. Anyway I'm looking at my changes and just noticed that at one point upstream source is using "qintptr" and not "int", like I thought). (DeviceListener.h line 41) I'm not completely sure what this implies, but I'm now sure that I did not correctly understand that line of code. I'm going to post a new patch as soon as I get some code that at least compiles :)
I tested various permutation with littel success. I'm running out of ideas. I posted what I got upstream as a draft pull request, let's see if upstream developers can help. https://github.com/keepassxreboot/keepassxc/pull/10736
Created attachment 250605 [details] patch v2 Hi, I'm attaching a new patch, following a new strategy, as suggested by upstream developers in the github Pull Request. As before, this compiles in poudriere for me, but I'm unable to test the modified code myself. I' d like to get some feedback about this before submitting the update upstream. Thanks in advance!
(In reply to Guido Falsi from comment #12) Sorry for the late reply. I haven't tested it with usb keys. The whole process is stuck when I was trying to open a key db file. I will try to provide more information.
(In reply to Li-Wen Hsu from comment #18) This is strange though, I have touched no code with relation to opening files. My changes are strictly in the code setting up and cleaning up USB listeners.
(In reply to Guido Falsi from comment #19) I suppose it's the same in context. Shouldn't KeePassXC list USB devices on opening DB to provide you a list of hardware keys if any?
(In reply to Anton Saietskii from comment #20) Not sure, the code to setup listeners is separated from the code to actually open the USB devices (which is the code that reacts to those listeners). BTW If this patch does not work I am mostly out of ideas. Maybe a backtrace of the failure could help, Could give me some hint on what is going wrong. This is a strange issue, because linux libusb is not really different from ours, so I don't understand why upstream is using "int" or qintptr things for the handles. I suspect GCC is much more permissive about such things. In the end the upstream code is simply passing around pointers, so I'm not sure what is wrong with it, once the code compiles.
(In reply to Guido Falsi from comment #21) Let's review once more my comment #13. We have libusb_hotplug_callback_handle_struct. Upstream does not have struct, but there's libusb_hotplug_callback_handle [0], which is just int. We have libusb_hotplug_callback_handle also -- not int, but a pointer to struct instead (if I understood code correctly). I don't really understand how messing with typecasts should help here. If I got your changes correctly -- you're trying to write a single integer to a structure which resembles one-way linked list, contains 4 integers, etc. (Of course, I may be and likely am completely wrong here.) [0]: https://github.com/libusb/libusb/blob/fef78a96e37936f16c10c43c9a220683f7c2ff74/libusb/libusb.h#L2282
(In reply to Anton Saietskii from comment #22) Hi, Well, my patch is not working so yes, I'm definitely wrong, this was easy to admit :) But, I'm not sure it is as easy as you put it to tell why I am wrong. There are two things you point out I need to address. One is, it looks to me you use the term "typecasting" inappropriately. In the context of C++ "type casting" means a very specific thing, for example what you see in [1] at the subtitle "type casting", otherwise we have implicit conversions. In my first patch I was not doing any of that, I'm actually trying to avoid any kind of type casting or type conversion when compiling on FreeBSD with it. This new patch does this because upstream suggested I use "qintptr" to represent any kind of pointer, but our USB functions take libusb_hotplug_callback_handle (aka struct libusb_hotplug_callback *) and clang will not accept that with an explicit typecast. [2] In the first version of the patch I was doing no type casting, only some type declaration games. Then regarding "libusb_hotplug_callback_handle" you point out correctly, that is is an int on Linux. On FreeBSD it is a pointer to a structure. What I'm doing (or trying to do, I could have made mistakes) is trying to pass around only the pointer, I never access the "pointed at" struct (if I do, please point me to the line of code where I do that, because I'm not aware of it and would like to learn where I'm wrong). As far as I know I'm not putting anything in the struct, nor trying to squeeze the struct in a pointer, I'm only moving around the pointer to it, never actually touching it. A pointer is just a number, as long as the architecture addressing is (these days mostly 64 bits) pointing to an (hopefully) valid memory address where the structure is. So a pointer CAN be passed around like an int with no problems. It is a very similar object. BTW it happens that "int" length is not mandated, but on i386 both ints and pointers happen to be 32 bit long universally. OTOH on amd64 they mostly are 32 bit for ints and 64 bits for pointers, but since int length is not mandated, how long an in is is defined by the compiler. libusb_hotplug_register_callback takes a pointer to an handle (so a pointer to a pointer to the struct) so it can fill the struct and give us back the pointer to it. libusb_hotplug_deregister_callback just takes the handle so it can clean it up. I never access what is pointed at by the pointer, just move it around, or at least, that's the objective. Unluckily it is not working. I'm clearly doing something wrong, but I have no idea what. I'd like your comment to shed some light on this, but it is not, unluckily. [1] https://cplusplus.com/doc/tutorial/typecasting/ [2] Not being a C++ expert I may have made a mistake here, using C style casts for pointers, I need to study the "static_cast" construct a little more.
Hi all, After some more experimenting I'm admitting I'm out of ideas on how to fix this code. My last work on this is at https://github.com/keepassxreboot/keepassxc/pull/10736 I hope someone there comes out with a clever idea we can implement.
Created attachment 250875 [details] patch v3 Hi again, I'd like to take another swing at this. I've prepared a new patch with a slightly different approach, trying to use our native type inside the wrapper methods, and casting only for the external interface. After this I'll be completely out of ideas. @lwhsu can you test this one again? Maybe we get a little more lucky? Thanks in advance!
(In reply to Guido Falsi from comment #25) Thanks and sorry that I still cannot spend too much time on this lately. I've just done a quick test but still no luck with me. I will try to squeeze some time to provide more clue.
Created attachment 250893 [details] bapt's approach
I am far from being an expert about C++, the main diff with Guido's version is the use of reinterpret_cast instead of static_case
(In reply to Baptiste Daroussin from comment #27) Seems wrong -- no actual patch there.
Created attachment 250909 [details] bapt's approach now with the real patches
Created attachment 250910 [details] bapt's approach even better if play correctly with git diff --staged
Sorry for hijacking, and sorry for not providing more details/patches... In version 2.7.6 the follwing features don't work here: - "Lock databases when session is locked or lid is closed" and even more important - 'Clear clipboard', no matter what timeout I define. I built with ===> The following configuration options are available for keepassxc-2.7.6: AUTOTYPE=on: Auto-type passwords in input fields BROWSER=on: Browser integration with KeePassXC-Browser FDOSECRETS=on: freedesktop.org secrets service support KEESHARE=on: Sharing integration with KeeShare NETWORKING=on: Networking support (e.g. for downloading website icons) SSHAGENT=on: SSH agent support TEST=off: Build and/or run tests YUBIKEY=on: YubiKey support 'Here' means I'm launching xfce4-session-4.18.3 from lightdm-1.32.0 on xorg-server-21.1.13 with xscreensaver-6.08. Dependencies of keepassxc are installed of course, but maybe not all the usual extra packages KDE or gnome do install as dependency. Can someone please check if these two features work in other xfce4-sessions resp. any other X11 setup? Users might rely on these security related features. I remember that I once was astonished that at least the clear-clipboard worked some time ago - can't tell when/what versions, sorry, too many page misses... Thanks, -harry
Hi all, any news with this? Has bapt patch been tested? If this is still failing, the only way ahead is by getting a backtrace with a DEBUG binary of the crash, so we have an idea where to look exactly. Is anyone with a USB key willing to provide this?
Created attachment 251529 [details] patch v4 (from upstream) Hi, Upstream developer phoerious provided a patch. I'm attaching port patches including it. This one compiles and runs fine for me, but I have no USB key to test with. Can you please provide feedback here or directly upstream? Thanks!
(In reply to Guido Falsi from comment #34) Followup to notice that the patch from upstream developer I sent has been merged upstream: https://github.com/keepassxreboot/keepassxc/commit/f4b91c17a9bcaf465382ee8f10e08005dd97cbf9
(In reply to Guido Falsi from comment #35) Please remind me — but we still don't know if it actually works?
(In reply to Anton Saietskii from comment #36) We don't. I performed a quick test suggested by upstream [1], and got a lockup in place of a crash. But this could indicate a more fundamental problem with our implementation of libusb. I suspect the implication of the bugs Ed Maste linked here is that most of the problem is coming from the aging libusb implementation in base and there is little that can be done in keepassxc (a consumer of it) to resolve the issue. [1] https://github.com/keepassxreboot/keepassxc/pull/10736#issuecomment-2175522078