rusticata / x509-parser

X.509 parser written in pure Rust. Fast, zero-copy, safe.
Other
206 stars 67 forks source link

certificate: fix lifetime signature for subject_alt_names getter #151

Closed BiagioFesta closed 7 months ago

BiagioFesta commented 9 months ago

Let us consider the following code:

use x509_parser::certificate::X509Certificate;
use x509_parser::extensions::GeneralName;
use x509_parser::prelude::FromDer;

fn foo(data: &[u8]) -> Vec<GeneralName> {
    X509Certificate::from_der(data)
        .unwrap()
        .1
        .subject_alternative_name()
        .unwrap()
        .unwrap()
        .value
        .general_names
        .clone()
}

Before the patch (does not compile)

error[E0515]: cannot return value referencing temporary value
  --> examples/my.rs:6:5
   |
6  | //     X509Certificate::from_der(data)
7  | ||         .unwrap()
   | ||_________________- temporary value created here
8  | |          .1
9  | |          .subject_alternative_name()
...  |
13 | |          .general_names
14 | |          .clone()
   | |_________________^ returns a value referencing data owned by the current function

After the patch (does compile)

No error as .clone() should produce a new vector bound to 'a

Additional Notes

BiagioFesta commented 8 months ago

Yeah, I saw your PR, that's why I wrote:

Added another commit to fix clippy, just for sake of green CI. Feel free to skip that commit

cpu commented 8 months ago

Yeah, I saw your PR, that's why I wrote:

Fair, and I quoted what you wrote in my reply linking #147 but since your text didn't link to the clippy fix PR, and the commit you added isn't cherry-picked from that branch, it wasn't clear from your text that you saw it.

In either case it sounds like we're on the same page and need maintainer action to move either branch forward :+1:

BiagioFesta commented 8 months ago

Yeah it seems we are a little bit stuck here. Waiting for maintainer action. :)

Btw, very sorry: it did not come up to my mind I could cherry-pick your commit considering they are 2 different remotes. I am going to try to do it

cpu commented 8 months ago

Yeah it seems we are a little bit stuck here. Waiting for maintainer action. :)

I sent an email to the primary maintainer this morning based on #150. Fingers crossed we can get things moving :crossed_fingers:

Btw, very sorry: it did not come up to my mind I could cherry-pick your commit considering they are 2 different remotes.

np!

BiagioFesta commented 8 months ago

cherry-picked proper commit for try_fold hint

SergioBenitez commented 8 months ago

Please also re-merge https://github.com/rusticata/x509-parser/pull/141 and https://github.com/rusticata/asn1-rs/pull/22#issuecomment-1507155046 which was undone in https://github.com/rusticata/x509-parser/commit/853d7ee1de14bd57c65e6b6493e9a4655740e490.

chifflier commented 8 months ago

Hi, I'm sorry for letting this wait so long. I'm catching back on PRs. I think I need to merge #147 first, which could break (or require at least a rebase) of others - including this one Thank you for your PR

chifflier commented 7 months ago

Applied, thanks