Make "into" patterns safer

[jsha]

When building a server config or a client config, we use the Rust "into" pattern- we start with an object of one type (*rustls_server_config_builder), and we consume it in order to produce an object of another type (*rustls_server_config). That means for that object's lifecycle, there are two ways out:

• Call rustls_server_config_builder_free to free it early (for instance if there was an error).
• Call rustls_server_config_builder_build to turn it into a *rustls_server_config.

If the user does both of these, it's undefined behavior (accessing uninitialized memory). If the user does neither of these, it's a memory leak. This sort of requirement to do exactly one of two things, in all paths through your code, introduces a lot of potential for mistakes. In the spirit of safety, we'd like to particularly minimize the potential for mistakes that result in undefined behavior.

Proposal: For objects that get consumed as part of their lifecycle, instead of storing them as Box, store them as Box<Option>[^1]. When the Foo in consumed, take it out of the Option. Document that foo_free must always be called, regardless of whether the Foo was consumed. If any methods are called on a foo after it has been consumed, return a new rustls_result code "AlreadyConsumed". For methods that don't return rustls_result, return a default value (false, 0, NULL, etc), as we already do for panics and null pointer inputs.

This takes the potential for undefined behavior and reduces it to a potential for memory leaks, and a small potential for incorrect application behavior if you call methods on a consumed object and rely on the default values returned. The cost is one extra word of storage and an extra check in each method on a consumable object. I think that cost should be pretty negligible.

/cc @icing @tgeoghegan @djc for feedback

[^1]: This does not participate in the Null Pointer Optimization, and we can't do Option<Box> instead. But that's fine.

[djc]

Seems reasonable to me!

[icing]

tl;dr: +1

The builder objects, as you explained, have a special state. Typical objects go from created+init to used to freed. The builder has the additional converted into and free is no longer allowed.

Adding an additional, internal indirection, so that the Rust side can detect invalid free calls seems like a very good idea. Especially since the costs are very minor.

Another option entirely is often pursued in frameworks with a Factory design. Which is created, configured and then churns out build instanced as long as one wants. And the factory always needs to be freed.

This something turns - in place - into another thing is a unique Rust feature, facilitated by its absolute knowledge of memory use and writability. No language without full, automated memory management can do that. Which explains why it is not straightforward to map the design pattern.

[cpu]

I think we can close this? All of the builders are using this pattern now as far as I can tell.