ghost commented 2 years ago

Proposal

Summary and problem statement

Create a new #[deprecated_safe] attribute allowing a function to be marked unsafe in a backwards compatible fashion.

Motivation, use-cases, and solution sketches

This is being proposed specifically to resolve the env::set_var/remove_var unsoundness. The same issue has previously occurred with CommandExt::before_exec, and this fix could retroactively be applied there as well.

The new attribute would eliminate the need to deprecate an unsound function and create an identical duplicate, other than having a different name and with unsafe added. The unsound function would instead be marked unsafe as it originally should have been, with #[deprecated_safe] added to indicate that its "safeness" was deprecated. This avoids the need to come up with a "good" alternate name, which may not always be available. It's also useful to avoid duplicates of set_var/remove_var in case these are ever able to be made safe again in the future.

[deprecated_safe] would act as an escape hatch anywhere that existing code would error due to the unsafe being added, and would instead issue a "safeness deprecated" lint. The affected function would have the unsafe modifier added and behave just like any other unsafe function, aside from this escape hatch behavior. Applying the attribute to a function that isn't marked unsafe should not be allowed.

The signature of e.g. set_var would thus change like so:

#[stable(feature = "env", since = "1.0.0")]
pub fn set_var<K: AsRef<OsStr>, V: AsRef<OsStr>>(key: K, value: V)

to:

#[stable(feature = "env", since = "1.0.0")]
#[deprecated_safe(since = "1.99.0", reason = "bla")]
pub unsafe fn set_var<K: AsRef<OsStr>, V: AsRef<OsStr>>(key: K, value: V)

The attribute could potentially accept an edition specifier, after which the lint would become an error/deny-by-default. The attribute could potentially be made applicable to other places like traits as well, if the need arises in the future. This is out of scope for this MCP, however, as it is only intended to be applied to functions.

Note: Are edition changes still required to always be auto-fixable? If the attribute supports specifying an edition after which the missing unsafe becomes an error, I don't see how we could ever wrap existing code in an unsafe block automatically. Personally I think requiring a manual fix is fine since we are talking about unsoundness.

This attribute could be made available for use for by all libraries as well, however the MCP only focuses on what's needed for set_var/remove_var. Because the attribute would initially be unstable, it could be used internally by libstd to fix set_var/remove_var before addressing any broader questions that would need to be answered before making this a publicly available stable attribute.

Note: Discussion on the zulip stream seems potentially amenable to putting this on a route to stability, if accepted. If that's the case it would make sense to include things like trait Bla {} becoming #[deprecated_safe] unsafe trait Bla {} in the design up front, instead of as a future possibility. Whether this attribute is useful outside of libstd (since libraries can making breaking changes), is questionable however.

The following places would break when adding unsafe after the fact to a function, but would instead lint with #[deprecated_safe] applied.

Note: This list may be incomplete, these are just the places I currently know about.

Call sites
```
env::set_var("TEST", "test");
```

Function pointer coercions

let set_var_fn_ptr: fn(OsString, OsString) = env::set_var;

Fn trait family coercions

let set_var_fn_impl: Box<dyn Fn(OsString, OsString)> = Box::new(env::set_var);
let mut set_var_fnmut_impl: Box<dyn FnMut(OsString, OsString)> = Box::new(env::set_var);
let set_var_fnonce_impl: Box<dyn FnOnce(OsString, OsString)> = Box::new(env::set_var);

Example output

The lint messages here are obviously very TODO, but this is an example of what the output might look like. All of these examples would previously have been errors when adding unsafe to set_var, before the use of the new attribute.

Proof-of-concept exploratory changes

I have explored making these known places work in a proof-of-concept fashion here: https://github.com/skippy10110/rust/commits/rustc_deprecated_safe

However, I'm unfamiliar with rustc and this is my first contribution, so the particular approaches taken may not be correct.

Call sites

This is a fairly straightforward change to the MIR unsafety checker in check_unsafety.rs. Modelled after the UNSAFE_OP_DEPRECATED_SAFE lint, a new UnsafetyViolationKind is added for DeprecatedSafe. The main change is the following to visit_terminator:

TerminatorKind::Call { ref func, .. } => {
    let func_ty = func.ty(self.body, self.tcx);
    let sig = func_ty.fn_sig(self.tcx);
    if let hir::Unsafety::Unsafe = sig.unsafety() {
        // NEW #[deprecated_safe] code here
        if let ty::FnDef(func_id, _) = func_ty.kind() && self.tcx.has_attr(*func_id, sym::deprecated_safe) {
            self.require_unsafe(
                UnsafetyViolationKind::DeprecatedSafe,
                UnsafetyViolationDetails::CallToUnsafeFunction,
            )
        } else {
            self.require_unsafe(
                UnsafetyViolationKind::General,
                UnsafetyViolationDetails::CallToUnsafeFunction,
            )
        }
    }

Note that the new THIR unsafety checker will also need to be updated. But this should be fairly straightforward.

Function pointer coercions

In coercion.rs the change is modelled after the existing code to coerce a "fn" pointer to an "unsafe fn" pointer. A new PointerCast::DeprecatedSafeFnPointer is added, the opposite of PointerCast::UnsafeFnPointer, and then coerce_from_safe_fn is modified to take advantage of this when the deprecated_safe attribute is present:

let is_deprecated_safe = if let ty::FnDef(def_id, _) = *a.kind() && self.tcx.has_attr(def_id, sym::deprecated_safe) {
    true
} else {
    false
};
let InferOk { value, obligations: o2 } = self.coerce_from_safe_fn(
    a_fn_pointer,
    a_sig,
    b,
    |unsafe_ty| {
        vec![
            Adjustment {
                kind: Adjust::Pointer(PointerCast::ReifyFnPointer),
                target: a_fn_pointer,
            },
            Adjustment {
                kind: Adjust::Pointer(PointerCast::UnsafeFnPointer),
                target: unsafe_ty,
            },
        ]
    },
    simple(Adjust::Pointer(PointerCast:ReifyFnPointer)),
    // NEW #[deprecated_safe] code here
    is_deprecated_safe,
    |safe_ty| {
        vec![
            Adjustment {
                kind: Adjust::Pointer(PointerCast::ReifyFnPointer),
                target: a_fn_pointer,
            },
            Adjustment {
                kind: Adjust::Pointer(PointerCast::DeprecatedSafeFnPointer),
                target: safe_ty,
            },
        ]
    },
)?;

fn coerce_from_safe_fn<F, G, H>(
    ...,
    is_deprecated_safe: bool,
    to_safe: H,
) -> CoerceResult<'tcx>
where
    F: FnOnce(Ty<'tcx>) -> Vec<Adjustment<'tcx>>,
    G: FnOnce(Ty<'tcx>) -> Vec<Adjustment<'tcx>>,
    H: FnOnce(Ty<'tcx>) -> Vec<Adjustment<'tcx>>,
{
    if let ty::FnPtr(fn_ty_b) = b.kind() {
        if let (hir::Unsafety::Normal, hir::Unsafety::Unsafe) =
            (fn_ty_a.unsafety(), fn_ty_b.unsafety())
        {
            let unsafe_a = self.tcx.safe_to_unsafe_fn_ty(fn_ty_a);
            return self.unify_and(unsafe_a, b, to_unsafe);
        }
        // NEW #[deprecated_safe] code here
        else if let (hir::Unsafety::Unsafe, hir::Unsafety::Normal) =
            (fn_ty_a.unsafety(), fn_ty_b.unsafety()) && is_deprecated_safe
        {
            self.tcx().struct_span_lint_hir(
                ...
            );

            let safe_a = self.tcx.unsafe_to_safe_fn_ty(fn_ty_a);
            return self.unify_and(safe_a, b, to_safe);
        }
    }
    self.unify_and(a, b, normal)
}

Fn trait family coercions

These are the proof-of-concept changes I'm the least certain about, by far. In candidate_assembly.rs, assemble_closure_candidates is modified to add the escape hatch here:

// Provide an impl for suitable functions, rejecting `#[target_feature]` functions (RFC 2396).
ty::FnDef(def_id, _) => {
    let is_deprecated_safe = self.tcx().has_attr(def_id, sym::deprecated_safe);

    if let ty::FnSig {
        unsafety: hir::Unsafety::Normal,
        abi: Abi::Rust,
        c_variadic: false,
        ..
    } = self_ty.fn_sig(self.tcx()).skip_binder()
    {
        if self.tcx().codegen_fn_attrs(def_id).target_features.is_empty() {
            candidates
                .vec
                .push(FnPointerCandidate { is_const: self.tcx().is_const_fn(def_id), is_deprecated_safe: false });
        }
    }
    // NEW #[deprecated_safe] code here
    else if let ty::FnSig {
        unsafety: hir::Unsafety::Unsafe,
        abi: Abi::Rust,
        c_variadic: false,
        ..
    } = self_ty.fn_sig(self.tcx()).skip_binder() && is_deprecated_safe
    {
        if self.tcx().codegen_fn_attrs(def_id).target_features.is_empty() {
            // TODO this is_dummy check is very suspicious, but otherwise a lint is emitted for Fn() FnMut() and FnOnce(), not just the actual coercion being done
            if !obligation.cause.span.is_dummy() {
                self.tcx().struct_span_lint_hir(
                    ...
                );
            }

            candidates
                .vec
                .push(FnPointerCandidate { is_const: self.tcx().is_const_fn(def_id), is_deprecated_safe: true });
        }
    }
}

The if !obligation.cause.span.is_dummy() strikes me as especially suspicious, see the comment in the code above. This lint will also only be emitted at the first place the coercion is needed, not subsequent places. E.g. this will only output a single lint:

let set_var_fn_impl1: Box<dyn Fn(OsString, OsString)> = Box::new(env::set_var);
let set_var_fn_impl2: Box<dyn Fn(OsString, OsString)> = Box::new(env::set_var);

However, this is fixed by modifying candidate_from_obligation to also emit the lint when the cached candidate is used. This strikes me as even more suspicious:

if let Some(c) =
    self.check_candidate_cache(stack.obligation.param_env, cache_fresh_trait_pred)
{
    // NEW #[deprecated_safe] code here
    // TODO i need to learn how to write this match/if let properly :)
    match c {
        Ok(Some(SelectionCandidate::FnPointerCandidate { is_deprecated_safe, .. }))
            if is_deprecated_safe =>
        {
            let obligation = stack.obligation;
            if !obligation.cause.span.is_dummy() {
                self.tcx().struct_span_lint_hir(
                    ...
                );
            }
        }
        _ => {}
    }

    debug!(candidate = ?c, "CACHE HIT");
    return c;
}

Also, just going by the name "candidate" in candidate_from_obligation alone, I wonder if these candidates might not even get used in certain scenarios and thus a false positive lint would be emitted.

Links and related work

Main IRLO discussion thread around the env unsoundness: https://internals.rust-lang.org/t/synchronized-ffi-access-to-posix-environment-variable-functions/15475 My initial idea for something like #[deprecated_safe]: https://internals.rust-lang.org/t/synchronized-ffi-access-to-posix-environment-variable-functions/15475/56 jhpratt describes the idea in more technical detail: https://internals.rust-lang.org/t/synchronized-ffi-access-to-posix-environment-variable-functions/15475/58

Future deprecation of env::{set, remove}_var: https://github.com/rust-lang/rust/pull/92365 Alternative to this MCP, Introduce unsafe methods for mutating environment: https://github.com/rust-lang/rust/pull/94619 Prior occurrence of this same issue, std::os::unix::process::CommandExt::before_exec should be unsafe: https://github.com/rust-lang/rust/issues/39575

Discussion thread on Zulip where I was working through implementing a proof-of-concept: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/deprecated_safe.20attribute

Initial people involved

Owner: @skippy10110
Liaison: @pnkfelix

What happens now?

This issue is part of the lang-team initiative process. Once this issue is filed, a Zulip topic will be opened for discussion, and the lang-team will review open proposals in its weekly triage meetings. You should receive feedback within a week or two.

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

rustbot commented 2 years ago

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

pnkfelix commented 2 years ago

@rustbot second

this is owned by @skippy10110 . I (@pnkfelix) will be liaison.

pnkfelix commented 2 years ago

@skippy10110 created tracking issue https://github.com/rust-lang/rust/issues/94978 for this, so I think we can close this issue and move discussion to the Pull Requests linked there.

(I'm hesitant to create a distinct zulip stream for each one of these MCPs, but if there is enough people interacting on a given topic, then we should make a zulip stream at that time.)

rust-lang / lang-team

Add #[deprecated_safe] attribute to allow functions be be marked unsafe in a backwards compatible fashion #147

Proposal

Summary and problem statement

Motivation, use-cases, and solution sketches

Example output

Proof-of-concept exploratory changes

Call sites

Function pointer coercions

Fn trait family coercions

Links and related work

Initial people involved

What happens now?