oxidecomputer / management-gateway-service

Crates shared between MGS in omicron and its agent task in hubris
Mozilla Public License 2.0
3 stars 3 forks source link

We can't use zerocopy 0.6.4 with hubris yet #123

Closed andrewjstone closed 1 year ago

andrewjstone commented 1 year ago

Dependabot just pulled in 0.6.4 of zerocopy, which indeed is useful and fixes a soundness hole.

Hubris has zerocopy@0.6.1. Bumping the hubris version to 0.6.4 results in the newer zerocopy pulling in zerocopy-derive@0.6.4 instead of zerocopy-derive@0.3.1 that is pulled in with zerocopy@0.6.1. Now, this wouldn't be much of a problem, except that zerocopy-derive@0.6.4 pulls in syn@2 instead of syn@1which is a major change, with syn@1 being used by a number of hubris crates. We using workspace dependencies for everything and so at the end of doing a simple version bump for zerocopy in hubris Cargo.toml I get the fun error:

 Compiling task-jefe-api v0.1.0 (/Users/ajs/oxide/hubris/task/jefe-api)
error[E0369]: binary operation `!=` cannot be applied to type `Fields`
  --> lib/derive-idol-err/src/lib.rs:44:21
   |
44 |         if v.fields != syn::Fields::Unit {
   |            -------- ^^ ----------------- Fields
   |            |
   |            Fields

If I try to also bump syn to v2I get the following errors:

  hubris git:(master) ✗ cargo xtask dist app/rot-carrier/app.toml
    Updating crates.io index
    Updating git repository `https://github.com/oxidecomputer/management-gateway-service`
    Finished dev [optimized + debuginfo] target(s) in 27.95s
     Running `target/debug/xtask dist app/rot-carrier/app.toml`
building crate task-jefe
   Compiling derive-idol-err v0.1.0 (/Users/ajs/oxide/hubris/lib/derive-idol-err)
error[E0369]: binary operation `!=` cannot be applied to type `Fields`
  --> lib/derive-idol-err/src/lib.rs:44:21
   |
44 |         if v.fields != syn::Fields::Unit {
   |            -------- ^^ ----------------- Fields
   |            |
   |            Fields

error[E0615]: attempted to take value of method `path` on type `&&Attribute`
  --> lib/derive-idol-err/src/lib.rs:91:27
   |
91 |             .filter(|s| s.path.segments[0].ident == "idol")
   |                           ^^^^ method, not a field
   |
help: use parentheses to call the method
   |
91 |             .filter(|s| s.path().segments[0].ident == "idol")
   |                               ++

error[E0609]: no field `tokens` on type `&Attribute`
  --> lib/derive-idol-err/src/lib.rs:93:18
   |
93 |             if s.tokens.is_empty() {
   |                  ^^^^^^ unknown field
   |
   = note: available fields are: `pound_token`, `style`, `bracket_token`, `meta`

error[E0609]: no field `tokens` on type `&Attribute`
  --> lib/derive-idol-err/src/lib.rs:99:24
   |
99 |             for t in s.tokens.clone() {
   |                        ^^^^^^ unknown field
   |
   = note: available fields are: `pound_token`, `style`, `bracket_token`, `meta`

Some errors have detailed explanations: E0369, E0609, E0615.
For more information about an error, try `rustc --explain E0369`.
error: could not compile `derive-idol-err` due to 4 previous errors
Error: failed to build jefe

Caused by:
    command failed, see output for details
andrewjstone commented 1 year ago

I believe the right thing to do here is indeed to move the hubris code to syn@2 before pulling in MGS updates. However, it's unclear to me right now how hard this may be. We may end wanting to back out the dependabot update of zerocopy depending upon urgency/priorities.

I ran into this while trying to pull #121 into hubris and I expect @mkeeter to run into the same thing when he tries to integrate #116 into hubris.

andrewjstone commented 1 year ago

The issue actually already existed with 9b39c775a3530d5b58cd208606c6e508216a60f1, which we also haven't pulled into hubris yet. To move along with my testing for #121, I temporarily reverted back to zerocopy@0.6.1 for now.

jgallagher commented 1 year ago

hubris can stay on syn@1 (moving to syn@2 is still probably right at some point, but it doesn't have to be forced by this issue). The error you're running into is because derive-idol-err needs the extra-traits syn1 feature but didn't enable it; this previously worked because zerocopy-derive 0.3.1 enabled the feature for us.

https://github.com/oxidecomputer/hubris/pull/1524 fixes the missing feature and bumps to the latest main MGS branch.

andrewjstone commented 1 year ago

Thank you @jgallagher