Add support for "unknown" bits.

[drmikehenry]

The nix crate uses bitflags heavily, as it provides a type-safe way of dealing
with the many different types of flags in the system. In most cases, the bit
flag definitions are specified by the system, and the nix project then provides
bitflags-based definitions for these flags. This leads to a robustness problem
as described below.

The bitflags structure will not allow "unknown" bit flag values to be stored.
All possible flag values must be known at compile time. Once an executable is
compiled, there is no way for it to gracefully handle system extensions that
define new flag values. Because many of these values are read from system
calls, the program can receive bit flag values it did not know about at compile
time, causing unnecessary errors for the previously correct program.

The nix issue linked below shows an example where this behavior of bitflags
leads to a less robust program that can't handle system extensions made after
the program was compiled:
nix-rust/nix#1102

For a hypothetical case, imagine a set of system-defined flags that can be read
from the system and written back after modification. To set SOME_FLAG, the
following C code might be used:

int flags = get_system_flags();
flags = flags | SOME_FLAG;
set_system_flags(flags);

This correctly handles any possible combinations of bits the operating system
might supply, even if those bit flags weren't defined when the C program was
compiled. Using bitflags, it's not possible to get the same level of
future-proofing; either from_bits() is used, causing an error when unknown
bits are provided by the system, or from_bits_truncate() is used, throwing
away these unknown bits and silently corrupting the state of the flags.

When the authoritative source of flag definitions is within the program itself,
it makes sense to prohibit unknown flag values in bitflags structures; but when
the bit flag definitions evolve in a backward-compatible fashion but
independently from the program, the only way to robustly handle
post-compile-time bit flag extensions is to allow unknown bits into a bitflags
structure.

This pull request's purpose is to add a third method of converting from bits
into a bitflags structure that accepts arbitrary bits without error or
truncation:

pub const fn from_bits_unknown(bits: $T) -> $BitFlags {
    $BitFlags { bits }
}

With this extension, a bitflags structure can represent any possible set of bits
the system deems valid. Once created, the structure retains its type safety
benefits and allows modification using the subset of bit flag definitions known
to the program at compile time. Unless the programmer uses
from_bits_unknown(), there is now way for unknown bits to creep into the
structure; for cases where it's important to allow for the unknown bits, the use
of this function stands as a clear marker to the reader.

[KodrAus]

Hi @drmikehenry 👋

This looks the same as #174 and I'm still not convinced it's an appropriate addition to bitflags as it muddies the waters about what's actually valid and what's not. We certainly shouldn't change the semantics of the existing bits method to surface invalid patterns.

This seems like a real concern for the nix crate though so I'm happy to explore what we can do to enable some measure of future-proofing.

[drmikehenry]

I'm sorry for the duplication of pull requests. @jabedude and I discussed an early version of this idea, but due to a miscommunication I didn't realize that Josh had submitted a pull request on the topic. I read through the open issues but I now see that pull requests don't show up in the list of issues.

The bitflags crate provides a nice abstraction over an integer that holds flag values. I agree that only well-defined flag values should be permitted into the integer underlying a bitflags structure. When the flag definitions are internal to the program, it's possible to define this set of valid flags at compile time, and bitflags currently handles that situation well. But in the case of a foreign function interface (FFI) like the nix crate provides, the set of valid flags can't be nailed down at compiled time because it's determined by an authority outside the program itself, where the set of valid flags may be extended at any time in the future.

With this view of the meaning of valid flags, it seems natural to me that the set of valid flags is divided into those that are known at compiled time and those that aren't (or, alternatively, those which have been named at compiled time and those which are unnamed or anonymous). If we assume that only valid flags will be present in the underlying integer, then the bits() function retains its current semantics of returning that integer which represents the current set of valid flags. The purpose of this pull request is to allow these anonymous yet valid flags to be stored in the underlying integer alongside other named flags, and to be extracted in the usual way via bits() when being passed through an FFI. Given the nature of this use case, it's not possible to lay down rules at compile time to vet the validity of flags; instead, it's necessarily to rely on the bits returned from the FFI as being correct. The intent of from_bits_unkown() is to be the single way for such valid bits to enter the bitflags structure. Once code review has determined that this function is being called correctly using bits from the system, then those bits are now trusted to be valid and there is no concern with bits() returning these valid bits.

Having a completely separate function (badly named bits_of_both_kinds() for the moment) for returning the complete set of valid bits (both known and unknown) has drawbacks. It seems likely that users would continue to use bits() where they ought to be using bits_of_both_kinds(), and it would be hard find this kind of bug (since it would appear to work with all named valid bits and might not be easily tested with anonymous bits values that aren't currently valid in the FFI). I even hesitated a bit adding bits_known() and bits_unknown(), though I thought their names should be enough to indicate a return of only a subset of the bits. In all of the cases I can imagine where it's reasonable to allow anonymous bits into the structure, there's no reason to restrict which bits can come back out of the structure; indeed, such bits were allowed in under the premise that they are equally valid with any of the named bits.

It may be that a different name for from_bits_unknown() (and unknown bits in general) would better convey what's going on. I also considered making the function unsafe, mainly because people are already expecting to look closely at unsafe calls to make sure they are used correctly.

Adding bits that are unknown/unnamed/anonymous (any better names?) seems like a natural extension to bitflags, and one that fixes a significant drawback for crates like nix that can't authoritatively define the set of valid flags at compile time. The downside is that for other use cases where the set of valid flags is completely determined at compile time, it's possible for someone to misuse from_bits_unknown() and get bad behavior. I'd hoped that the name of the function would be telling enough to prevent its misuse; perhaps a better name would suffice. Or maybe making the function unsafe, or adding a crate-wide opt-in feature gate to enable the use of this function, would be enough to allay any concerns. Allowing the change to be crate-wide would empower end users to work around problems like I found with the nix crate, without requiring such a crate to change.

Thanks for considering this issue, and again I apologize for not noticing the earlier pull request.

[KodrAus]

Thanks for the explanation @drmikehenry!

Ok, I can appreciate your perspective, so I think we could explore something like from_bits_unchecked.

Just as a stream-of-consciousness, in order to determine whether or not this function should be unsafe I think we need to lay down some semantics for what makes a BitFlags(T) different from a T. I think we should require that BitFlags(T) always contains a valid bit pattern, where that pattern will usually be determined by the the closed set of valid flags (in your case it isn't). In that sense, I think from_bits_unchecked should be unsafe, so bits remains unchanged.

Do you have a need to distinguish the bits you 'know' from the bits you don't?

[drmikehenry]

I like the word "unchecked". I think it gives the right impression about what's going on.

To me, the big win BitFlags provides is the type safety you get between two different BitFlags(T) types (say, BF1 and BF2) that would otherwise both be T. Once flags are safely inside a BitFlags structure, you can't mistakenly pass them to functions or assign them to variables where a different BitFlags structures is expected, nor perform logic operations using flags defined in a different structure or using plain values of type T. These protections are great, and they eliminate a large class of hard-to-detect bugs.

For example, the C library function fcntl() mentioned previously has this signature:

  int fcntl(int fd, int cmd, ... /* arg */ );

Depending on the value of cmd, arg may or may not be used, and the meaning of arg may change completely. fcntl supports two pairs of confusingly similar cmd values for getting and setting flags on file descriptors:

• F_GETFD - get the file descriptor flags
• F_SETFD - set the file descriptor flags to arg
• F_GETFL - get the file status flags
• F_SETFL - set the file status flags to arg

File descriptor flags and file status flags have different definitions and interpretations; here are two such definitions taken from the fcntl.h header on Linux:

#define FD_CLOEXEC  1   // a file descriptor flag for use with F_GETFD/F_SETFD
#define O_WRONLY    1   // a file status flag for use with F_GETFL/F_SETFL

When trying to set FD_CLOEXEC on a file descriptor, it's easy to intend the first line below but to actually write the second (error checking is omitted for brevity):

fcntl(fd, F_SETFD, fcntl(fd, F_GETFD) | FD_CLOEXEC);  // intended
fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) | FD_CLOEXEC);  // mistake!

The second line uses F_SETFL and F_GETFL where F_SETFD and F_GETFD were intended. Given the flag values, this code actually corresponds to these two equivalent lines:

fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) | 1);
fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) | O_WRONLY);  // actual call

The mistaken call has the effect of setting the file's write-only bit, which will typically succeed and might easily be a no-op, while failing to set the close-on-exec bit, causing a subtle leak of a file descriptor into an exec'ed process that might easily go unnoticed.

Use of BitFlags in scenarios like the above prevents this kind of mistake. The only hole in the protection comes from constructing a BitFlags structure from type T containing raw bit values. The from_bits() and from_bits_truncate() functions do what they can to protect this boundary, but they only ensure the values aren't out-of-range. The programmer still has to ensure the bits come from a trustworthy source (for example, from the correct system call).

In answer to your question, in general I don't have any use case for querying a BitFlags structure about "known" or "unknown" bits, and this discussion has convinced me that the bits_known() and bits_unknown() functions are probably attractive nuisances that should be removed. It's not hard for BitFlags users to perform logic with the all() function to isolate known and unknown bits should it be necessary. This simplifies the pull request significantly, and it nicely dodges the question of naming these two subclasses of flag bits.

I've updated the pull request along the above lines. Let me know what you think.

[asomers]

Don't forget to update the CHANGELOG, too.

[drmikehenry]

Thanks, @asomers; once @KodrAus is satisfied with the structure, naming, etc., in the pull request, I can propose some wording for the CHANGELOG.md (assuming that's desired).

[KodrAus]

Thanks all for your patience here! I'm just dropping a line here to let you know that I'll give this the proper review it deserves next week 🙂 I'm just buried in some other things at the moment.

[drmikehenry]

@KodrAus Take your time - I've got a work-around in the meantime, and it's worth making sure you are satisfied with the approach.

[KodrAus]

Ok, thanks for your patience @jabedude and @drmikehenry!

This looks good to me.

[KodrAus]

bors r+

[bors]

Build failed

• continuous-integration/travis-ci/push

[drmikehenry]

I don't really know how bors works, and I don't have access to the report results for the failure above. I'm assuming therefore that there's nothing I should be doing here, but please let me know if the above failure indicates I need to change the pull request somehow.

[KodrAus]

bors retry

[bors]

Build failed

• continuous-integration/travis-ci/push

[KodrAus]

bors retry

[KodrAus]

Alrighty, I'll give this one more try and if bors is still unhappy I'll merge manually :)

[bors]

Build failed

• continuous-integration/travis-ci/push

[notriddle]

https://travis-ci.com/bitflags/bitflags/jobs/238038279

[drmikehenry]

@KodrAus Thanks for the merge; bitflags 1.2.0 is working fine for me :-)