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

Destabilize the `Error::type_id` function #60787

Merged
merged 2 commits into from May 14, 2019

Conversation

Projects
None yet
8 participants
@alexcrichton
Copy link
Member

commented May 13, 2019

This commit destabilizes the Error::type_id function in the standard library.
This does so by effectively reverting #58048, restoring the #[unstable]
attribute. The security mailing list has recently been notified of a
vulnerability relating to the stabilization of this function. First stabilized
in Rust 1.34.0, a stable function here allows users to implement a custom
return value for this function:

struct MyType;

impl Error for MyType {
fn type_id(&self) -> TypeId {
    // Enable safe casting to `String` by accident.
    TypeId::of::<String>()
}
}

This, when combined with the Error::downcast family of functions, allows
safely casting a type to any other type, clearly a memory safety issue! A
formal announcement has been made to the security mailing list as well as the blog

This commit simply destabilizes the Error::type_id which, although breaking
for users since Rust 1.34.0, is hoped to have little impact and has been deemed
sufficient to mitigate this issue for the stable channel. The long-term fate of
the Error::type_id API will be discussed at #60784.

Destabilize the `Error::type_id` function
This commit destabilizes the `Error::type_id` function in the standard library.
This does so by effectively reverting #58048, restoring the `#[unstable]`
attribute. The security mailing list has recently been notified of a
vulnerability relating to the stabilization of this function. First stabilized
in Rust 1.34.0, a stable function here allows users to implement a custom
return value for this function:

    struct MyType;

    impl Error for MyType {
	fn type_id(&self) -> TypeId {
	    // Enable safe casting to `String` by accident.
	    TypeId::of::<String>()
	}
    }

This, when combined with the `Error::downcast` family of functions, allows
safely casting a type to any other type, clearly a memory safety issue! A
security announcement will be shortly posted to the security mailing list as
well as the Rust Blog, and when those links are available they'll be filled in
for this PR as well.

This commit simply destabilizes the `Error::type_id` which, although breaking
for users since Rust 1.34.0, is hoped to have little impact and has been deemed
sufficient to mitigate this issue for the stable channel. The long-term fate of
the `Error::type_id` API will be discussed at #60784.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 13, 2019

r? @KodrAus

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

@pietroalbini

This comment has been minimized.

Copy link
Member

commented May 13, 2019

@bors r+ p=500

@bors

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

📌 Commit 3db667a has been approved by pietroalbini

@ubsan

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Is there something wrong with making it an unsafe trait?

@sfackler

This comment has been minimized.

Copy link
Member

commented May 13, 2019

@ubsan Making the Error trait unsafe would break literally every error type in the ecosystem.

@ubsan

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

@sfackler oh, I see, right; you want it to only be unsafe to write the function

@ubsan

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

So it'd be better to have something on the order of

unsafe trait TypeInfo {
  fn type_id(&self) -> TypeId;
}

impl<T: ?Sized> TypeInfo for T {
  fn type_id(&self) -> TypeId {
    TypeId::of::<Self>()
  }
}

trait Error : TypeInfo {}
@bors

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

⌛️ Testing commit 3db667a with merge 291ea75...

bors added a commit that referenced this pull request May 13, 2019

Auto merge of #60787 - alexcrichton:error-type-id-destabilize-master,…
… r=pietroalbini

Destabilize the `Error::type_id` function

This commit destabilizes the `Error::type_id` function in the standard library.
This does so by effectively reverting #58048, restoring the `#[unstable]`
attribute. The security mailing list has recently been notified of a
vulnerability relating to the stabilization of this function. First stabilized
in Rust 1.34.0, a stable function here allows users to implement a custom
return value for this function:

    struct MyType;

    impl Error for MyType {
	fn type_id(&self) -> TypeId {
	    // Enable safe casting to `String` by accident.
	    TypeId::of::<String>()
	}
    }

This, when combined with the `Error::downcast` family of functions, allows
safely casting a type to any other type, clearly a memory safety issue! A
formal announcement has been made to the [security mailing list](https://groups.google.com/forum/#!topic/rustlang-security-announcements/aZabeCMUv70) as well as [the blog](https://blog.rust-lang.org/2019/05/13/Security-advisory.html)

This commit simply destabilizes the `Error::type_id` which, although breaking
for users since Rust 1.34.0, is hoped to have little impact and has been deemed
sufficient to mitigate this issue for the stable channel. The long-term fate of
the `Error::type_id` API will be discussed at #60784.
@bors

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

💔 Test failed - checks-travis

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

The job arm-android of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[01:49:34] test string::test_str_clear ... ok
[01:49:34] test string::test_str_truncate ... ok
[01:49:34] test string::test_str_truncate_invalid_len ... ok
[01:49:34] test string::test_str_truncate_split_codepoint ... ok
[01:49:34] died due to signal 11
[01:49:34] 
[01:49:34] 
[01:49:34] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "arm-linux-androideabi" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "alloc" "--"
[01:49:34] expected success, got: exit code: 3
---
travis_time:end:08504cb5:start=1557796268113303392,finish=1557796268127153888,duration=13850496
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0234c5fe
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:294798ee
travis_time:start:294798ee
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:02e1fc00
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@xfix

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

The proposed TypeInfo trait doesn't have to be unsafe, as it cannot be implemented (similarly to how std::any::Any is a safe trait because it's not possible to provide a manual implementation). That said, for TypeInfo to be usable in such a manner, TypeInfo has to be implemented for all types while the type_id would be only available for Self: 'static (and really, if it could be done without doing this, we could as well use std::any::Any instead of creating a new TypeInfo trait).

use std::any::TypeId;
use std::fmt::{Debug, Display};

trait TypeInfo {
    fn type_id(&self) -> TypeId
    where
        Self: 'static;
}

impl<T: ?Sized> TypeInfo for T {
    fn type_id(&self) -> TypeId
    where
        T: 'static,
    {
        TypeId::of::<Self>()
    }
}

trait Error: Debug + Display + TypeInfo {
    fn description(&self) -> &str {
        "description() is deprecated; use Display"
    }
    fn cause(&self) -> Option<&dyn Error> {
        self.source()
    }
    fn source(&self) -> Option<&(dyn Error + 'static)> {
        None
    }
}

Also, one thing I feel like Rust is missing is being able to declare a trait method as unsafe to implement. std::io::Read::initializer sorta tries to do this by marking the method as unsafe, but the method isn't unsafe, implementing it with non-default-implementation is, so it's not quite accurate.

@pietroalbini

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@bors retry

@xfix can we keep the discussion in #60784? Thanks ❤️

@bors

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

⌛️ Testing commit 3db667a with merge 4423351...

bors added a commit that referenced this pull request May 14, 2019

Auto merge of #60787 - alexcrichton:error-type-id-destabilize-master,…
… r=pietroalbini

Destabilize the `Error::type_id` function

This commit destabilizes the `Error::type_id` function in the standard library.
This does so by effectively reverting #58048, restoring the `#[unstable]`
attribute. The security mailing list has recently been notified of a
vulnerability relating to the stabilization of this function. First stabilized
in Rust 1.34.0, a stable function here allows users to implement a custom
return value for this function:

    struct MyType;

    impl Error for MyType {
	fn type_id(&self) -> TypeId {
	    // Enable safe casting to `String` by accident.
	    TypeId::of::<String>()
	}
    }

This, when combined with the `Error::downcast` family of functions, allows
safely casting a type to any other type, clearly a memory safety issue! A
formal announcement has been made to the [security mailing list](https://groups.google.com/forum/#!topic/rustlang-security-announcements/aZabeCMUv70) as well as [the blog](https://blog.rust-lang.org/2019/05/13/Security-advisory.html)

This commit simply destabilizes the `Error::type_id` which, although breaking
for users since Rust 1.34.0, is hoped to have little impact and has been deemed
sufficient to mitigate this issue for the stable channel. The long-term fate of
the `Error::type_id` API will be discussed at #60784.
@bors

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

💥 Test timed out

@pietroalbini

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@bors retry

@bors

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

⌛️ Testing commit 3db667a with merge f59c71e...

bors added a commit that referenced this pull request May 14, 2019

Auto merge of #60787 - alexcrichton:error-type-id-destabilize-master,…
… r=pietroalbini

Destabilize the `Error::type_id` function

This commit destabilizes the `Error::type_id` function in the standard library.
This does so by effectively reverting #58048, restoring the `#[unstable]`
attribute. The security mailing list has recently been notified of a
vulnerability relating to the stabilization of this function. First stabilized
in Rust 1.34.0, a stable function here allows users to implement a custom
return value for this function:

    struct MyType;

    impl Error for MyType {
	fn type_id(&self) -> TypeId {
	    // Enable safe casting to `String` by accident.
	    TypeId::of::<String>()
	}
    }

This, when combined with the `Error::downcast` family of functions, allows
safely casting a type to any other type, clearly a memory safety issue! A
formal announcement has been made to the [security mailing list](https://groups.google.com/forum/#!topic/rustlang-security-announcements/aZabeCMUv70) as well as [the blog](https://blog.rust-lang.org/2019/05/13/Security-advisory.html)

This commit simply destabilizes the `Error::type_id` which, although breaking
for users since Rust 1.34.0, is hoped to have little impact and has been deemed
sufficient to mitigate this issue for the stable channel. The long-term fate of
the `Error::type_id` API will be discussed at #60784.
@bors

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: pietroalbini
Pushing f59c71e to master...

@bors bors added the merged-by-bors label May 14, 2019

@bors bors merged commit 3db667a into rust-lang:master May 14, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.