Make `from_bits` in `bitflags!` safe; add `from_bits_truncate`

Previously, the `from_bits` function in the `std::bitflags::bitflags`
macro was marked as unsafe, as it did not check that the bits being
converted actually corresponded to flags.

This patch changes the function to check against the full set of
possible flags and return an `Option` which is `None` if a non-flag bit
is set. It also adds a `from_bits_truncate` function which simply zeros
any bits not corresponding to a flag.

This addresses the concern raised in https://github.com/mozilla/rust/pull/13897

diff --git a/src/libnative/io/file_unix.rs b/src/libnative/io/file_unix.rs
index c2b69483fa1e0d4e206f5de4ddb10216a155478b..046d2875d553101700a56b970ca2c6a72098e52c 100644 (file)
--- a/src/libnative/io/file_unix.rs
+++ b/src/libnative/io/file_unix.rs
@@ -493,9 +493,7 @@ fn gen(_stat: &libc::stat) -> u64 { 0 }
     io::FileStat {
         size: stat.st_size as u64,
         kind: kind,
-        perm: unsafe {
-            io::FilePermission::from_bits(stat.st_mode as u32) & io::AllPermissions
-        },
+        perm: io::FilePermission::from_bits_truncate(stat.st_mode as u32),
         created: mktime(stat.st_ctime as u64, stat.st_ctime_nsec as u64),
         modified: mktime(stat.st_mtime as u64, stat.st_mtime_nsec as u64),
         accessed: mktime(stat.st_atime as u64, stat.st_atime_nsec as u64),

diff --git a/src/libnative/io/file_win32.rs b/src/libnative/io/file_win32.rs
index d721e1d67f1b99bf9d85f79c50c6868b144f3251..3222c912dd0857fe6f3afea505dd7b47c6d7eef8 100644 (file)
--- a/src/libnative/io/file_win32.rs
+++ b/src/libnative/io/file_win32.rs
@@ -492,9 +492,7 @@ fn mkstat(stat: &libc::stat) -> io::FileStat {
     io::FileStat {
         size: stat.st_size as u64,
         kind: kind,
-        perm: unsafe {
-          io::FilePermission::from_bits(stat.st_mode as u32)  & io::AllPermissions
-        },
+        perm: io::FilePermission::from_bits_truncate(stat.st_mode as u32),
         created: stat.st_ctime as u64,
         modified: stat.st_mtime as u64,
         accessed: stat.st_atime as u64,

diff --git a/src/librustuv/file.rs b/src/librustuv/file.rs
index 06271e61ce7a0f9f72d4361a835ee4e3584a62da..12636a3c490ad0ca64daafe083f8ff9dea0eb5d7 100644 (file)
--- a/src/librustuv/file.rs
+++ b/src/librustuv/file.rs
@@ -285,9 +285,7 @@ fn to_msec(stat: uvll::uv_timespec_t) -> u64 {
         FileStat {
             size: stat.st_size as u64,
             kind: kind,
-            perm: unsafe {
-                io::FilePermission::from_bits(stat.st_mode as u32) & io::AllPermissions
-            },
+            perm: io::FilePermission::from_bits_truncate(stat.st_mode as u32),
             created: to_msec(stat.st_birthtim),
             modified: to_msec(stat.st_mtim),
             accessed: to_msec(stat.st_atim),

diff --git a/src/libstd/bitflags.rs b/src/libstd/bitflags.rs
index 32f9bc1173b2dec7e148fb22a049762a3e694d87..163ccd22552d36fed65495580b6c4a5c1b7798a0 100644 (file)
--- a/src/libstd/bitflags.rs
+++ b/src/libstd/bitflags.rs
@@ -136,10 +136,20 @@ pub fn bits(&self) -> $T {
                 self.bits
             }
 
-            /// Convert from underlying bit representation. Unsafe because the
-            /// bits are not guaranteed to represent valid flags.
-            pub unsafe fn from_bits(bits: $T) -> $BitFlags {
-                $BitFlags { bits: bits }
+            /// Convert from underlying bit representation, unless that
+            /// representation contains bits that do not correspond to a flag.
+            pub fn from_bits(bits: $T) -> ::std::option::Option<$BitFlags> {
+                if (bits & !$BitFlags::all().bits()) != 0 {
+                    ::std::option::None
+                } else {
+                    ::std::option::Some($BitFlags { bits: bits })
+                }
+            }
+
+            /// Convert from underlying bit representation, dropping any bits
+            /// that do not correspond to flags.
+            pub fn from_bits_truncate(bits: $T) -> $BitFlags {
+                $BitFlags { bits: bits } & $BitFlags::all()
             }
 
             /// Returns `true` if no flags are currently stored.
@@ -209,6 +219,7 @@ fn not(&self) -> $BitFlags {
 
 #[cfg(test)]
 mod tests {
+    use option::{Some, None};
     use ops::{BitOr, BitAnd, Sub, Not};
 
     bitflags!(
@@ -231,9 +242,21 @@ fn test_bits(){
 
     #[test]
     fn test_from_bits() {
-        assert!(unsafe { Flags::from_bits(0x00000000) } == Flags::empty());
-        assert!(unsafe { Flags::from_bits(0x00000001) } == FlagA);
-        assert!(unsafe { Flags::from_bits(0x00000111) } == FlagABC);
+        assert!(Flags::from_bits(0) == Some(Flags::empty()));
+        assert!(Flags::from_bits(0x1) == Some(FlagA));
+        assert!(Flags::from_bits(0x10) == Some(FlagB));
+        assert!(Flags::from_bits(0x11) == Some(FlagA | FlagB));
+        assert!(Flags::from_bits(0x1000) == None);
+    }
+
+    #[test]
+    fn test_from_bits_truncate() {
+        assert!(Flags::from_bits_truncate(0) == Flags::empty());
+        assert!(Flags::from_bits_truncate(0x1) == FlagA);
+        assert!(Flags::from_bits_truncate(0x10) == FlagB);
+        assert!(Flags::from_bits_truncate(0x11) == (FlagA | FlagB));
+        assert!(Flags::from_bits_truncate(0x1000) == Flags::empty());
+        assert!(Flags::from_bits_truncate(0x1001) == FlagA);
     }
 
     #[test]