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

Keepassx fails to compile on FreeBSD due to libusb implementation differences #10736

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

madpilot78
Copy link

On FreeBSD the libusb_hotplug_register_callback() function uses a pointer to a struct as an handle.

I'm changing the methods to use this as expected by the system library.

This now compiles fine but fails at runtime.

Not sure why this patch is not working and I must admit I do not understand what is being done using qintptr in

typedef qintptr Handle;
.

This has been noticed here: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=277652

The failures with my patch are described here: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=277652#c10

I do not have a yubikey so I cannot do testing myself.

Thanks in advance for any help!

Testing strategy

As said, with this changes the source compiles on FreeBSD, and the resulting binary works, except when exercising the modified code.

Type of change

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

@madpilot78 madpilot78 marked this pull request as draft May 11, 2024 21:18
@madpilot78 madpilot78 changed the title Keepassx fails ti compile on FReeBSD due to libusb implementation differences Keepassx fails ti compile on FreeBSD due to libusb implementation differences May 11, 2024
Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

Instead of adding all the ifdefs, you should cast libusb_hotplug_callback_handle to qintptr instead. This should already be an opaque pointer type that needs no additional logic.

@@ -102,7 +111,7 @@ int DeviceListenerLibUsb::registerHotplugCallback(bool arrived, bool left, int v
return handle;
}

void DeviceListenerLibUsb::deregisterHotplugCallback(int handle)
Copy link
Member

@phoerious phoerious May 12, 2024

Choose a reason for hiding this comment

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

This is actually a bug and should be qintptr aka Handle.

void deregisterAllHotplugCallbacks();

signals:
void devicePlugged(bool state, void* ctx, void* device);

private:
void* m_ctx;
QSet<int> m_callbackHandles;
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@madpilot78 madpilot78 changed the title Keepassx fails ti compile on FreeBSD due to libusb implementation differences Keepassx fails to compile on FreeBSD due to libusb implementation differences May 12, 2024
@madpilot78
Copy link
Author

@phoerious Thanks for the feedback!

So, if I understand correctly, there is an existing bug in the code that uses ints where it should be using qintptr. This makes some sense to me, now I can understand why qintptr was used to start with.

I'll do some testing hoping I can make sense of all this.

Thanks again for your suggestions.

@phoerious
Copy link
Member

On a normal 64 bit system it shouldn't make a difference, but with smaller word lengths it may.

@madpilot78
Copy link
Author

@phoerious Problem is, the code as is fails to compile on FreeBSD. FreeBSD is using recent clang by default, which tends to be less permissive about this kind of casts.

Putting the correct types in the correct place should be enough to make it compile and work then.

On FreeBSD the libusb_hotplug_register_callback() function uses a
pointer to a struct as an handle.

I'm changing the methods to use this as expected by the system library.

This now compiles fine but fails at runtime.
@madpilot78
Copy link
Author

Hi again,

I experimented a little but made little progress.

I uploaded what I have now, following suggestion to use qintptr.

This fails to compile. If I use classic C style casts in place of static_cast it does compile but fails at runtime.

I'm asking for help on how to fix this. My limited C++ knowledge has already reached its limit.

Thanks in advance.

src/gui/osutils/nixutils/DeviceListenerLibUsb.cpp Outdated Show resolved Hide resolved
@@ -84,7 +84,7 @@ int DeviceListenerLibUsb::registerHotplugCallback(bool arrived, bool left, int v
return 0;
},
that,
&handle);
&static_cast<libusb_hotplug_callback_handle>(handle));
Copy link
Member

Choose a reason for hiding this comment

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

Both your build and runtime error is here, you shouldn't reference the output of the static cast directly. Make the handle variable an int, then static cast the return call.

error: cannot take the address of an rvalue of type 'libusb_hotplug_callback_handle' (aka 'int')
        &static_cast<libusb_hotplug_callback_handle>(handle));
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Copy link
Author

Choose a reason for hiding this comment

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

Why int?

Please keep in mind that I'm trying to fix this in FreeBSD. FreeBSD has its own implementation of libusb. AFAIK other BSDs do too, but I have not checked.

FreeBSD's libusb_hotplug_callback_handle is defined as [1]:

typedef struct libusb_hotplug_callback_handle_struct *libusb_hotplug_callback_handle;

so the error I'm getting is:

/wrkdirs/usr/ports/security/keepassxc/work/keepassxc-2.7.8/src/gui/osutils/nixutils/DeviceListenerLibUsb.cpp:87:10: error: cannot cast from type 'Handle' (aka 'long long') to pointer type 'libusb_hotplug_callback_handle' (aka 'libusb_hotplug_callback_handle_struct *')
   87 |         &static_cast<libusb_hotplug_callback_handle>(handle));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/wrkdirs/usr/ports/security/keepassxc/work/keepassxc-2.7.8/src/gui/osutils/nixutils/DeviceListenerLibUsb.cpp:110:77: error: cannot cast from type 'Handle' (aka 'long long') to pointer type 'libusb_hotplug_callback_handle' (aka 'libusb_hotplug_callback_handle_struct *')
  110 |     libusb_hotplug_deregister_callback(static_cast<libusb_context*>(m_ctx), static_cast<libusb_hotplug_callback_handle>(handle));
      |                                                                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.

I'm not sure what I should cast to at this point, on FreeBSD.

If later Linux (or any other OS) requires a different code I will use #ifdefs to differentiate.

[1] https://github.com/freebsd/freebsd-src/blob/48edad2edf6eb7a539e40dad8e1f87e3fa4973fd/lib/libusb/libusb.h#L311

Copy link
Member

Choose a reason for hiding this comment

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

Either way your static cast is incorrect in this context. Slow clap for FreeBSD for a completely divergent API with the same exact function name.

Copy link
Author

Choose a reason for hiding this comment

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

Well I don't know how and why a different API evolved in FreeBSD, but I'm trying to understand why a 64bit number on a 64bit arch cannot be cast to a pointer. The memory representation of them is the same AFAIK.

What I'm trying to do is adapt the code in keepassxc for these function to pass around pointers in place of integers. Apart fro the different type (with a different size in most cases), the API is not that much different, so the basic concept should be possible to put in practice.

Copy link
Member

@phoerious phoerious May 23, 2024

Choose a reason for hiding this comment

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

On Ubuntu (libusb 1.0.26), libusb_hotplug_callback_handle is typedef'ed to int. FreeBSD has an entirely different implementation of the whole library.

I think the main problem is that libusb_hotplug_callback_handle itself is not a pointer type on FreeBSD. Instead, they chose to typedef only *libusb_hotplug_callback_handle, so you have to use it with *.

@droidmonkey droidmonkey added this to the v2.7.9 milestone May 22, 2024
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.

None yet

3 participants