1380270 - Add dlopen() version of libudev-sys

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, enhancement, P2)

Description

[Tim Taubert [:ttaubert] (inactive)]

Just like with the Gamepad code [1], we can't link against libudev on Linux. So we need a similar solution that uses dlopen() + dlsym() for the U2F Rust code.

[1] http://searchfox.org/mozilla-central/source/dom/gamepad/linux/udev.h

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1380270-Add-dlopen-version-of-libudev-sys.patch

I started with a fork of libudev-sys [1], that was generated by rust-bindgen, and then added the dlopen() and dlsym() code, just keeping the function signatures. Thus I wouldn't really call it a fork anymore... Let me know if you disagree.

The version number is 0.1.3 because we have to override libudev-sys:0.1.3 as required by the libudev package.

[1] https://github.com/dcuddeback/libudev-sys/blob/master/src/lib.rs

Comment 2

[J.C. Jones [:jcj] (he/they)]

We should put a README in the folder explaining why this override is here, why it's named what it is, and what would happen if the udev package changes upstream. :)

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Is this a review for a github repo or something? Or is this going in the tree somewhere?

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Agh hit save changes too soon. Anyways, I realize it's in dom/webauthn, I just don't see build files or linkage into the rest of our system with it and I'm not quite sure what the plan is for it.

Comment 5

[J.C. Jones [:jcj] (he/they)]

(In reply to Kyle Machulis [:qdot] [:kmachulis]  (if a patch has no decent commit message, automatic r-) from comment #4)
> Agh hit save changes too soon. Anyways, I realize it's in dom/webauthn, I
> just don't see build files or linkage into the rest of our system with it
> and I'm not quite sure what the plan is for it.

Basically: There's enough code that we're going to land it / review it in chunks, and then tie it into the build system. This is pretty stand-alone, but it's also weird to make this compile w/o landing other parts too, so it's just going to be dead for a few days. At least, that's our intent.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to J.C. Jones [:jcj] from comment #2)
> We should put a README in the folder explaining why this override is here,
> why it's named what it is, and what would happen if the udev package changes
> upstream. :)

That's definitely a good idea, will do that.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Kyle Machulis [:qdot] [:kmachulis] from comment #3)
> Is this a review for a github repo or something? Or is this going in the
> tree somewhere?

I actually started this as a GitHub repo but have removed it since, it's going in the tree anyway.

(In reply to Kyle Machulis [:qdot] [:kmachulis] from comment #4)
> Agh hit save changes too soon. Anyways, I realize it's in dom/webauthn, I
> just don't see build files or linkage into the rest of our system with it
> and I'm not quite sure what the plan is for it.

What J.C. said, we need this for our main Rust library which isn't quite ready to land yet, so this won't build until we actually land that too. I thought it would be a good idea to split the reviews and get that landed first.

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to J.C. Jones [:jcj] from comment #5)
> Basically: There's enough code that we're going to land it / review it in
> chunks, and then tie it into the build system. This is pretty stand-alone,
> but it's also weird to make this compile w/o landing other parts too, so
> it's just going to be dead for a few days. At least, that's our intent.

Hmm, ok. The problem is that I'm not really sure how this will tie into the rust side of our build system, and :ted, who I would usually bring in for that (as well as another set of eyes on these bindings, because he wrote the C bindings for gamepad), is on vacation until next Monday. Can these patches hang out a few days until I can make ni?/r? assignments to :ted?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Yeah, no problem at all. We're not in a rush to land this :)

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8885649 [details] [diff] [review]
0001-Bug-1380270-Add-dlopen-version-of-libudev-sys.patch

We need to late bind libudev-sys for the USB portions of USB U2F in rust. Ted, how do you want to deal with this in the rust build system?

Comment 11

[J.C. Jones [:jcj] (he/they)]

(In reply to Tim Taubert [:ttaubert] from comment #9)
> Yeah, no problem at all. We're not in a rush to land this :)

We're not in a rush, per say, but I really do want to have this in place before August for external testing...

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

We're getting closer to have the U2F Rust library in shape for review. Ted, can you please give us some feedback about this dependency soon?

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 8885649 [details] [diff] [review]
0001-Bug-1380270-Add-dlopen-version-of-libudev-sys.patch

Review of attachment 8885649 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the wait here. I don't see anything that looks bad here. A few questions:
1) Do you think we could potentially upstream this to libudev-sys / libudev as a cargo feature? It would be nice to not have to maintain this, and it might be useful for other people. Also, the fact that you are using a cargo override to replace libudev-sys isn't the best.
2) I've seen code like this in a different -sys crate, I bet there's room for either a helper crate for this pattern or adding support to bindgen to generate bindings that allow dlopening a library and defining dynamically-loaded function pointers like this.

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8885649 [details] [diff] [review]
0001-Bug-1380270-Add-dlopen-version-of-libudev-sys.patch

Review of attachment 8885649 [details] [diff] [review]:
-----------------------------------------------------------------

If ted's ok with it I'm ok with it.

ted: I'm not aware of any crates to generate for dlopen, but that's a good idea, maybe worth looking into in a followup?

Comment 15

[(not currently active) Ted Mielczarek]

Yeah. It just seems like the kind of thing that might be more generally useful.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> 1) Do you think we could potentially upstream this to libudev-sys / libudev
> as a cargo feature? It would be nice to not have to maintain this, and it
> might be useful for other people. Also, the fact that you are using a cargo
> override to replace libudev-sys isn't the best.

Yeah, we should eventually upstream it. JC filed bug 1395294.

> 2) I've seen code like this in a different -sys crate, I bet there's room
> for either a helper crate for this pattern or adding support to bindgen to
> generate bindings that allow dlopening a library and defining
> dynamically-loaded function pointers like this.

Yeah, it would be nice to make this easier. We have similar code in tree that can handle both:

http://searchfox.org/mozilla-central/source/media/libcubeb/cubeb-pulse-rs/pulse-ffi/src/ffi_funcs.rs

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a5f3c233e1b
Add dlopen() version of libudev-sys r=qdot,ted

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7a5f3c233e1b

Comment 19

[guigs [:guigs] PST (please needinfo me!)]

Hey team I am having a user mention that these are not turned on by default? 
https://twitter.com/jacobian/status/931319475093225472

Any ideas if that is just github?

Comment 20

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Landing Sheriff isn't gonna be able to help you much on that. Redirecting to :jcj.

Comment 21

[J.C. Jones [:jcj] (he/they)]

I replied [1]; U2F we're not planning to ship. Web Authentication is coming along nicely, and currently looks like 59/60 timeframe [2].

[1] https://twitter.com/jamespugjones/status/931324766782332928
[2] https://wiki.mozilla.org/Security/CryptoEngineering#WD-07_Updates