Frostie314159 / ieee80211-rs

A library implementing various aspects of the IEEE 802.11 standard.
Apache License 2.0
54 stars 5 forks source link

Lifetime issues when trying to return a reference to an element #4

Closed FeldrinH closed 3 months ago

FeldrinH commented 3 months ago

Since all the frame and element types have an explicit lifetime parameter, I was hoping that I could write a function that takes a reference to bytes and return a reference to an element. Something like this:

pub fn borrow_element<'a>(packet: &'a [u8]) -> Option<VendorSpecificElement<'a>> {
    let frame = packet.pread::<IEEE80211Frame>(0).unwrap();

    if let IEEE80211Frame::Management(mgmt) = frame {
        if let ManagementFrameBody::Beacon(beacon) = mgmt.body {
            if let Some(vendor_specific) = beacon.elements.get_first_element::<VendorSpecificRepr>() {
                return Some(vendor_specific);
            }
        }
    }

    None
}

Unfortunately this does not compile, because the lifetime of the element is for some reason tied to the lifetime of beacon.elements itselt, not the bytes contained within. Compiler error:

error[E0515]: cannot return value referencing local data `beacon.elements`
   --> src\read_frame.rs:174:24
    |
173 |             if let Some(vendor_specific) = beacon.elements.get_first_element::<VendorSpecificRepr>() {
    |                                            --------------- `beacon.elements` is borrowed here
174 |                 return Some(vendor_specific);
    |                        ^^^^^^^^^^^^^^^^^^^^^ returns a value referencing data owned by the current function

I believe the problem starts with this signature:

pub fn get_first_element<ElementType: ElementTypeRepr>(
    &'a self,
) -> Option<<<ElementType as ElementTypeRepr>::ElementType<'a> as Element>::ReadType<'a>>

For my snippet to work the lifetime of &self should not be tied to 'a:

pub fn get_first_element<ElementType: ElementTypeRepr>(
    &self,
) -> Option<<<ElementType as ElementTypeRepr>::ElementType<'a> as Element>::ReadType<'a>>

Taking a quick look at the code, it seems that the problem is relatively pervasive. A number of other functions also have the same issue or something similar. It would be nice if this could be fixed, to make the library more flexible.

PS: Taking a quick look at the code, it seems to me that this should be fixable simply by tweaking some lifetime parameters, with no other changes necessary.

Frostie314159 commented 3 months ago

Hi, thanks for the issue. I'm gonna reproduce it on my end, and it should be fixed soon. Did you test against the crates.io release or master? There were severe changes in that region, completely removing the type representations, greatly simplifying this API. I'll release a new version, once VHT is done.

FeldrinH commented 3 months ago

I tested 0.2.2 from crates.io. Latest master did not compile.

Frostie314159 commented 3 months ago

Oh yeah, I accidentally pushed the module declaration, before the module was done. This will be fixed in a few minutes, because I'm just finishing up the VHT elements.

Frostie314159 commented 3 months ago

Just pushed VHT support and CI goes through now. Sorry for the chaos.

FeldrinH commented 3 months ago

Currently away from my computer, but looking at the code the lifetime handling of elements hasn't changed much, so master should have the same issue.

Frostie314159 commented 3 months ago

Yeah it has, I'm working on it rn.

Frostie314159 commented 3 months ago

This should've been fixed by the latest commit. The solution is (surprisingly), to just use self instead of &self, which makes absolutely no difference performance wise, since ReadElements is just a slice internally. I added a test, for this. Let me know, if this resolved the issue for you.

FeldrinH commented 3 months ago

The issue with elements does seem to be fixed. ProbeRequestBody.ssid and BeaconLikeFrameBody.ssid also have a similar problem and there might be some more that I have missed. For those two it should be as simple as replacing &'a self with &self to decouple the borrow from the lifetime of the returned value.

Frostie314159 commented 3 months ago

I pushed another commit, which also resolves this for the SSID. Thanks a lot for helping catch this. This was my first real issue, apart from an incorrectly declared module and it makes me really happy to see someone use my library. I hope it's doing its job otherwise.

FeldrinH commented 3 months ago

Haven't done any heavy testing as I only need a couple of vendor specific elements from beacon and action frames, but so far it has been working fine. I wish you luck, the 802.11 standard is huge, but having a reliable and fast dissector in Rust is valuable.

Frostie314159 commented 3 months ago

Thanks, love to hear that. I wish you good luck, with your project, as well. I'm gonna close this issue as completed.