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

Provide opt-out feature=from_source to use serde_derive source #2580

Closed
wants to merge 10 commits into from

Conversation

pinkforest
Copy link
Contributor

@pinkforest pinkforest commented Aug 19, 2023

Closes #2538
Supercedes #2579

Follows my recommendation: #2579 (comment)

Current choice: Opt-Out by feature=from_source (current HEAD commit)

Triggers build from source if used with feature from_source via either serde or serde_derive

Alternative choice: Opt-Out by cfg(serde_derive_build = "source") (commit #6bccee)

Override via cfg(serde_derive_build = "source") to build from source

Has flaw re: cross compile as per @bjorn3 at #2580 (comment) due to Cargo limitation - but can be addressed via adocumentation as has been done via commit #23059f7

Alterative: Opt-In - My recommendation (commit #8d544)

Would make opt-in choice to use the precompiled binary with cfg(serde_derive_build = "precompiled") and leaving it to the top-level binary to use the precompiled blob if it explicitly opts in to - by default people will compile from source always.

cfg Approach

This also demonstrates the cfg based approach that frees dependency chain from messing up with the features in the middle leaving it to the top level binary as informed choice.

This is used elsewhere e.g.: https://github.com/dalek-cryptography/curve25519-dalek/tree/main/curve25519-dalek#manual-backend-override - that was result of this discussion: dalek-cryptography/curve25519-dalek#414 providing background why / how this approach has been used elsewhere.

Cc/ @sagudev I cherry picked

@pinkforest pinkforest changed the title Cfg override Provide opt-in cfg to use precompiled blob Aug 19, 2023
@simonsan
Copy link

How would I specify that I don't want the precompiled binary when I only pull in serde with the serde_derive feature like this?

serde = { version = "=1.0.171", features = ["serde_derive"] }

@pinkforest
Copy link
Contributor Author

pinkforest commented Aug 19, 2023

@simonsan

Please note that this PR proposes to make it opt-in only where the top-level binary would need to want the precompiled

By default you would get it built from the source if this PR is merged and leaves it to the top level bin to opt-in the binary.

If the maintainer wishes this to be opt-out instead then could always add a feature that signals the cfg override

EDIT: I further clarified at top - thanks for the q.

@kayabaNerve
Copy link

kayabaNerve commented Aug 19, 2023

I will repeat my comment from #2579.

I don't believe this adequately resolves #2538. This would still cause unknown, unsigned, and unverifiable executables to placed on any systems which use serde as a dependency. That's still a notable security risk.

While I'm obviously in favor of this, as it's a massive improvement, this still leaves an unverifiable executable sitting on the system.

I'd state the binary should only be downloaded on opt-in.

EDIT: What about a serde-derive-bin which has the binary, yet is optional and only downloaded under this cfg? That wouldn't reduce security from the current status quo, and would prevent anyone who doesn't opt-in from needing to download the bin.

@pinkforest
Copy link
Contributor Author

pinkforest commented Aug 19, 2023

@kayabaNerve it is opt-in ? it doesn't download or does it ? Or did I miss the download somewhere ? let me check

EDIT: Ah yes ofcourse I forgot it came from the published .crate itself and it is unavoidable ofcourse

(anyone has been able to push anything they like as part of the .crate as it's "a feature" and we can't solve that here today)

@oli-obk
Copy link
Member

oli-obk commented Aug 19, 2023

serde-derive-bin which has the binary

That would certainly make it more explicit and easier to reject in a crate graph. But it doesn't actually change too much, as afaik it will still be downloaded, just never built. Or at least show up in a Cargo.lock

@andyquinterom
Copy link

As someone who works in the medical field with Rust I want to express my support for this

@oli-obk
Copy link
Member

oli-obk commented Aug 19, 2023

Two things

  1. I don't think changing the default to opt-in will be something that we'll land right now. I rather think that starting out with making it opt out will be something we'll be able to land while figuring out the entire situation.
  2. Please document how to use this in the readme.

@CryZe
Copy link

CryZe commented Aug 19, 2023

I rather think that starting out with making it opt out will be something we'll be able to land while figuring out the entire situation.

Why? This feature didn't even exist until recently, is considered experimental and is widely considered a problem. This should be opt-in until the Rust community agrees as a whole that it is good (safe, reproducible, ...) enough to be opt-out.

@oli-obk
Copy link
Member

oli-obk commented Aug 19, 2023

Why?

Because this crate is owned by dtolnay and he may do with it as he wishes. My personal opinions on the topic are irrelevant. While I would prefer fundamental crates to have entire (funded!) teams working on them, that's not the reality.

I am reviewing this from the perspective of getting the PR landed.

@novacrazy
Copy link

Because this crate is owned by dtolnay and he may do with it as he wishes.

Serde has nearly 200 Million downloads on crates.io, and is a staple of the entire Rust ecosystem. It's downright disrespectful to the entire Rust community to say their wants and needs are less important just because the one and only benevolent dictator has the publish rights on crates.io. There's a good argument that this crate should be adopted by the Rust org to avoid such bottlenecks. At this scale the need to appeal to someone's feelings is irrelevant. We need this to be opt-in until the day things upstream are safe enough to guarantee prebuilt binaries will be safe, perhaps using projects like Watt.

@kayabaNerve
Copy link

@pinkforest While I agree anyone being able to include any file is a distinct issue, it being in a separate crate which isn't actually downloaded would solve that. I do see a comment claiming optional dependencies, even if unused, will appear in the crate graph and be downloaded though, which may void my comment's validity.

@oli-obk
Copy link
Member

oli-obk commented Aug 19, 2023

It's downright disrespectful to the entire Rust community to say their wants and needs are less important just because the one and only benevolent dictator has the publish rights on crates.io

I am not saying that. I am saying what the reality is. Not what I wish reality were.

@pinkforest
Copy link
Contributor Author

@oli-obk ok - I will add another commit to opt-out - can always re-base to previous commit if opt-in is chosen instead so choice will be with maintainers - I will also add doc how to use it - thanks

@pinkforest
Copy link
Contributor Author

New behaviour with cfg(serde_derive_build = "source")

Default - host x86_64-linux-gnu

$ cargo build
   Compiling serde_derive v1.0.183 (/home/foobar/code/serde/precompiled/serde_derive)
    Finished dev [unoptimized + debuginfo] target(s) in 0.16s

Override - host x86_64-linux-gnu

$ env RUSTFLAGS='--cfg serde_derive_build="source"' cargo build
   Compiling proc-macro2 v1.0.66
   Compiling unicode-ident v1.0.11
   Compiling quote v1.0.33
   Compiling syn v2.0.29
   Compiling serde_derive v1.0.183 (/home/foobar/code/serde/precompiled/serde_derive)
    Finished dev [unoptimized + debuginfo] target(s) in 3.17s

@pinkforest pinkforest changed the title Provide opt-in cfg to use precompiled blob Provide opt-out cfg to use serde_derive source Aug 19, 2023
@newpavlov
Copy link

newpavlov commented Aug 19, 2023

@simonsan

How would I specify that I don't want the precompiled binary when I only pull in serde with the serde_derive feature like this?

The most flexible option is to use:

serde = { version = "1.0.x, <1.0.172", features = ["derive"] }

where x is minimum version required by your project.

@oli-obk

But it doesn't actually change too much, as afaik it will still be downloaded, just never built. Or at least show up in a Cargo.lock

IIRC unused crates do not get downloaded during compilation, but, yes, they get mentioned in Cargo.lock.

From my experience, having a separate crate for pre-compiled version of the crate will be quite useful when dependencies get vendored. Even though it will be mentioned in Cargo.lock, we will not have a semi-random blob as part of our vendored source code.

@bjorn3
Copy link

bjorn3 commented Aug 19, 2023

It is really easy to accidentally override the rustflags and thus opt back into the precompiled version. RUSTFLAGS will override ~/.cargo/config.toml and a per-project .cargo/config.toml will override the global one. As such putting the opt-out once in your ~/.cargo/config.toml may accidentally use the precompiled binary again if the project you build uses .cargo/config.toml or you or the project needs RUSTFLAGS (eg in rust's build system)

Edit: Github was borked so I thought my comment was lost so I wrote it again. After I noticed that it wasn't gone I deleted my second comment.

README.md Outdated Show resolved Hide resolved
@bjorn3
Copy link

bjorn3 commented Aug 20, 2023

If serde_derive is used for a build dependency afaik with the v2 resolver it wouldn't get feature unified with serde_derive used by a non-build dependency or the root crate.

@@ -73,6 +73,11 @@ fn main() {
}
```

## Note about serde_derive Binary
Copy link
Contributor Author

@pinkforest pinkforest Aug 20, 2023

Choose a reason for hiding this comment

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

@oli-obk Sadly with either CARGO_ENCODED_RUSTFLAGS or given these .cargo/config neither work 😭

build.target

[build]
target = "wasm32-unknown-unknown"

[target.wasm32-unknown-unknown]
rustflags = ['--cfg=serde_derive_build="source"']

build.rustflags

[build]
rustflags = ['--cfg=serde_derive_build="source"']

Both set the options but it doesn't get seen at serde_derive procmacro level still

Copy link

Choose a reason for hiding this comment

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

To get rustflags into build script builds you need to use -Zhost-config and

[host]
rustflags = [...]

Copy link
Contributor Author

@pinkforest pinkforest Aug 20, 2023

Choose a reason for hiding this comment

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

Oh we have a winner! Thanks @Nemo157 ❤️

$ cargo -Zhost-config build
error: the -Zhost-config flag requires the -Ztarget-applies-to-host flag to be set
[foobar@localhost whatever]$ cargo -Ztarget-applies-to-host -Zhost-config build
   Compiling proc-macro2 v1.0.66
   Compiling unicode-ident v1.0.11
   Compiling whatever v0.1.0 (/home/foobar/code/whatever)
warning: "x86_64-unknown-linux-gnu"
   Compiling quote v1.0.33
   Compiling syn v2.0.29
   Compiling serde_derive v1.0.183 (/home/foobar/code/pushpush/serde/precompiled/serde_derive)
    Finished dev [unoptimized + debuginfo] target(s) in 2.84s
[foobar@localhost whatever]$ cargo -Ztarget-applies-to-host -Zhost-config build --target wasm32-unknown-unknown
   Compiling proc-macro2 v1.0.66
   Compiling unicode-ident v1.0.11
   Compiling whatever v0.1.0 (/home/foobar/code/whatever)
warning: "wasm32-unknown-unknown"
   Compiling quote v1.0.33
   Compiling syn v2.0.29
   Compiling serde_derive v1.0.183 (/home/foobar/code/pushpush/serde/precompiled/serde_derive)
    Finished dev [unoptimized + debuginfo] target(s) in 3.43s
[foobar@localhost whatever]$ cat .cargo/config
[host]
rustflags = ['--cfg=serde_derive_build="source"']

When were these -Z introduced ? 🤔 so we can document it as required version if using this opt-out ?

Copy link

Choose a reason for hiding this comment

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

I think using an unstable option for the opt-out is wrong. Also it still has the same issue of getting overriden by other ways to specify rustflags rather than appending to it.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for trying it out

Copy link
Member

Choose a reason for hiding this comment

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

It's fragile (or rather: easy to get wrong). Is there some solution to the resolver=2 problem?

Copy link
Contributor Author

@pinkforest pinkforest Aug 20, 2023

Choose a reason for hiding this comment

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

Good question need to build a test-case for it and prove it works I'll do that shortly.

EDIT: Yeah so I've replicated the resolver problem. Maybe we can just document this ?
This was the repro for it: https://github.com/pinkforest/serde_optout_unification_proof

Copy link
Member

Choose a reason for hiding this comment

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

Alright with me. Is there something that can be done to ensure build dependencies also have the feature enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems I didn't clear the target properly between my repro previously

and it seems I can't replicate the problem with feature unification

e.g. I don't see anything wrong with the feature approach ?

Copy link

@Nemo157 Nemo157 Aug 20, 2023

Choose a reason for hiding this comment

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

That's because it's a proc-macro crate, which is its own kind of dependency similar to a build-dependency, which is built for a single target: the host; so both the build-dep and normal-dep refer to the same proc-macro-dependency and their features get unified in it. A more accurate cargo tree would look like

top-bin v0.1.0 (/tmp/tmp.mNq6xeO1Ms/serde_optout_unification_proof/top-bin)
└── some-lib v0.1.0 (/tmp/tmp.mNq6xeO1Ms/serde_optout_unification_proof/some-lib)
    [proc-macro-dependencies]
    └── serde_derive v1.0.183 (proc-macro) (https://github.com/serde-rs/serde?rev=cb23de92e7f479139add329e8629eb3b179109f6#cb23de92)
        ├── proc-macro2 v1.0.66
        │   └── unicode-ident v1.0.11
        ├── quote v1.0.33
        │   └── proc-macro2 v1.0.66 (*)
        └── syn v2.0.29
            ├── proc-macro2 v1.0.66 (*)
            ├── quote v1.0.33 (*)
            └── unicode-ident v1.0.11
[build-dependencies]
└── some-build-lib v0.1.0 (/tmp/tmp.mNq6xeO1Ms/serde_optout_unification_proof/some-build-lib)
    [proc-macro-dependencies]
    └── serde_derive v1.0.183 (proc-macro) (https://github.com/serde-rs/serde?rev=cb23de92e7f479139add329e8629eb3b179109f6#cb23de92) (*)

EDIT: And by switching both dependencies to serde = { features = ["derive"] } instead of being direct you can see that there are two versions of serde built (because no (*)), one for host one for target, that both link against the same serde_derive (because (*)):

> cargo tree --features=from_source
warning: skipping duplicate package `serde_derive` found at `/home/nemo157/.cargo/git/checkouts/serde-1cbd136c2e06ab68/cb23de9/precompiled/serde_derive`
top-bin v0.1.0 (/tmp/tmp.mNq6xeO1Ms/serde_optout_unification_proof/top-bin)
└── some-lib v0.1.0 (/tmp/tmp.mNq6xeO1Ms/serde_optout_unification_proof/some-lib)
    └── serde v1.0.183 (https://github.com/serde-rs/serde?rev=cb23de92e7f479139add329e8629eb3b179109f6#cb23de92)
        [proc-macro-dependencies]
        └── serde_derive v1.0.183 (proc-macro) (https://github.com/serde-rs/serde?rev=cb23de92e7f479139add329e8629eb3b179109f6#cb23de92)
            ├── proc-macro2 v1.0.66
            │   └── unicode-ident v1.0.11
            ├── quote v1.0.33
            │   └── proc-macro2 v1.0.66 (*)
            └── syn v2.0.29
                ├── proc-macro2 v1.0.66 (*)
                ├── quote v1.0.33 (*)
                └── unicode-ident v1.0.11
[build-dependencies]
└── some-build-lib v0.1.0 (/tmp/tmp.mNq6xeO1Ms/serde_optout_unification_proof/some-build-lib)
    └── serde v1.0.183 (https://github.com/serde-rs/serde?rev=cb23de92e7f479139add329e8629eb3b179109f6#cb23de92)
        [proc-macro-dependencies]
        └── serde_derive v1.0.183 (proc-macro) (https://github.com/serde-rs/serde?rev=cb23de92e7f479139add329e8629eb3b179109f6#cb23de92) (*)

Co-authored-by: sagudev <16504129+sagudev@users.noreply.github.com>
Co-authored-by: Oliver Schneider <oli-obk@users.noreply.github.com>
@najtin

This comment was marked as resolved.

@Piturnah

This comment was marked as resolved.

With stable in non-cross compile context is still possible:

RUSTFLAGS='--cfg serde_derive_build="source"'

Co-authored-by: Nemo157 <github@nemo157.com>
@LuckyTurtleDev
Copy link

Hey, initially an opt-in version was pitched. Is opt-in off the table now?

I personally would also prefer an opt-in solution. But if any crate you used has opt-in to use the binary, you can not disable it anymore. So opt-out would probably the better solution.

@bjorn3
Copy link

bjorn3 commented Aug 20, 2023

Opt-in through RUSTFLAGS doesn't have that problem.

@pinkforest
Copy link
Contributor Author

pinkforest commented Aug 20, 2023

@bjorn3 fwiw - I just tested parked at commit #585482 the resolver="2" and it seems to work regardless ?

Repro here: https://github.com/pinkforest/serde_optout_unification_proof

Source by opt-out

top-bin]$ rm -rf ../target ; cargo build --features from_source
   Compiling proc-macro2 v1.0.66
   Compiling unicode-ident v1.0.11
   Compiling quote v1.0.33
   Compiling syn v2.0.29
   Compiling serde_derive v1.0.183 (/home/foobar/code/serde/precompiled/serde_derive)
   Compiling some-lib v0.1.0 (/home/foobar/code/serde_optout_unification_proof/some-lib)
   Compiling top-bin v0.1.0 (/home/foobar/code/serde_optout_unification_proof/top-bin)
    Finished dev [unoptimized + debuginfo] target(s) in 2.97s

Default precompiled

top-bin]$ rm -rf ../target ; cargo build
   Compiling serde_derive v1.0.183 (/home/foobar/code/serde/precompiled/serde_derive)
   Compiling some-lib v0.1.0 (/home/foobar/code/serde_optout_unification_proof/some-lib)
   Compiling top-bin v0.1.0 (/home/foobar/code/serde_optout_unification_proof/top-bin)
    Finished dev [unoptimized + debuginfo] target(s) in 0.33s

Anything I did wrong here to not reproduce the given problem with resolver = "2" ?

My preference certainly would be opt-in as well but will see what the decision here is given options provided.

This can be useful in target environments which require building
from the source.

The feature flag is provided both via serde_derive and serde.

Co-authored-by: sagudev <16504129+sagudev@users.noreply.github.com>
Co-authored-by: Oliver Schneider <oli-obk@users.noreply.github.com>
@AlexTMjugador
Copy link

I'd like to mention that the decision of using precompiled binaries for Linux x64 platforms is already causing significant ecosystem fragmentation. That might be useful to keep in mind for reviewing this PR: time-rs/time#611

@jnicholls
Copy link
Contributor

IMHO Opt-in for the precompiled binary should be the desired outcome. It is backwards compatible with the ecosystem's current serde dependency specifications, and its expectations before learning about the precompiled binary.

@Rusty-Weasel
Copy link

As followed this issue, just mention to care about upcoming cyber resillience act,
only/or by default just binary would drop out serde of projects.

@bjorn3
Copy link

bjorn3 commented Aug 20, 2023

@bjorn3 fwiw - I just tested parked at commit #585482 the resolver="2" and it seems to work regardless ?

Maybe it only applies to non-proc-macro deps? In any case great that it works.

@joshka
Copy link

joshka commented Aug 20, 2023

@AlexTMjugador wrote:

I'd like to mention that the decision of using precompiled binaries for Linux x64 platforms is already causing significant ecosystem fragmentation. That might be useful to keep in mind for reviewing this PR: time-rs/time#611

Regarding this - do you think it would be worth putting forward a PR that fully reverts the precompiled PR until this or #2579 land (or whatever is the real fix)? To me that approach seems like it will give some breathing room to work out the correct fix.

@AlexTMjugador
Copy link

Regarding this - do you think it would be worth putting forward a PR that fully reverts the precompiled PR until this or #2579 land (or whatever is the real fix)? To me that approach seems like it will give some breathing room to work out the correct fix.

I'd support that, but I'm not a serde repository maintainer to decide.

@joshka
Copy link

joshka commented Aug 20, 2023

I'd support that, but I'm not a serde repository maintainer to decide.

Yep - I understand that. I read your reply over in the time issue and figured you might have a reasonable perspective on this one way or another.

@faulesocke
Copy link

from_source should really be on by default, making it not include the binary in the default feature set.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

This code is no longer used since serde_derive 1.0.184 — would you be interested in sending a different PR to delete it? The precompiled directory and anything having to do with cfg(precompiled) in the rest of the code.

Thanks, I appreciate your diligence on this PR, and the dozens of commenters who helped brainstorm the possible approaches for an opt-out and opt-in implementation.

@pinkforest
Copy link
Contributor Author

Thanks will do - appreciate looking over - I can just override this existing branch in this PR no problem

@dtolnay
Copy link
Member

dtolnay commented Aug 21, 2023

Nice. I'd prefer a new PR to preserve the final working approach that was settled on in this one for opt-out.

@dtolnay dtolnay closed this Aug 21, 2023
@pinkforest
Copy link
Contributor Author

pinkforest commented Aug 21, 2023

Ok great valid point I agree ❤️

pinkforest added a commit to pinkforest/serde that referenced this pull request Aug 21, 2023
This PR phases out the precompiled per final consensus made in serde-rs#2580
i#
Fix a cfg
AlexTMjugador pushed a commit to ComunidadAylas/serde that referenced this pull request Aug 23, 2023
* Release 1.0.184

* Following consensus on: serde-rs#2580 (review)

This PR phases out the precompiled per final consensus made in serde-rs#2580
i#
Fix a cfg

* No need for slow macOS CI if there is no platform-specific code

* Add repro of issue 2591

    error[E0507]: cannot move out of `*__self` which is behind a shared reference
       --> test_suite/tests/test_remote.rs:210:10
        |
    210 | #[derive(Serialize, Deserialize)]
        |          ^^^^^^^^^
        |          |
        |          data moved here
        |          move occurs because `unrecognized` has type `ErrorKind`, which does not implement the `Copy` trait
        |
        = note: this error originates in the derive macro `Serialize` (in Nightly builds, run with -Z macro-backtrace for more info)
    help: consider borrowing here
        |
    210 | #[derive(&Serialize, Deserialize)]
        |          +

* Fix "cannot move out of `*self` which is behind a shared reference"

* Release 1.0.185

---------

Co-authored-by: David Tolnay <dtolnay@gmail.com>
Co-authored-by: pinkforest <36498018+pinkforest@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

using serde_derive without precompiled binary