161

I have a fairly complex maths library I'm working on, and I have discovered a nasty bug when client code uses auto. Halfway through creating a minimal reproductive case to ask a question about it, I realise I can reproduce something similar using the standard library alone. See this simple test case:

#include <vector>
#include <assert.h>

int main()
{
    std::vector<bool> allTheData = {true, false, true};

    auto boolValue = allTheData[1]; // This should be false - we just declared it.
    assert(boolValue == false);
    boolValue = !boolValue;
    assert(boolValue == true);

    assert(allTheData[1] == false); // Huh? But we never changed the source data! Only our local copy.
}

Live on Godbolt. (Fun fact: Clang actually optimises this to writing "7" - 3 true bits - and a call to __assert_fail.)

(Yes I know std::vector<bool> sucks - but in this case it's handy to create a minimum reproducible example that's only a few lines long) Here's a longer example that doesn't use std::vector<bool>, and uses a custom container type, with assignment and copy/move deleted, and still shows the problem.

I understand what is going on under the hood, there's a proxy class returned by operator[] meant to implement allTheData[1] = true and related functionality, the client code that is written as if it's reading the value is actually storing the proxy in boolValue, and then when the client later modifies what it thinks is a bool, the original source data is modified instead. TLDR: 'auto' copied the proxy.

The code did what the programmer told it to do, not what the programmer meant.

If the programmer wanted boolValue's changes to update the source data, they would've done auto& boolValue = ..., which works with operator[] implementations returning T&, but not those needing custom proxies which fake reference-like behavior.

All the copy and move constructors and both assignment operators for the proxy are declared private (have also tried = delete), but this bug isn't caught at compile time. The proxy is copied regardless of whether the copy constructor is deleted.

All the "fixes" I've found for this bug focus on the client part of the code. They're things like: "don't use auto", "cast to the underlying type", "access through a const ref", etc. These are all substandard fixes, once you discover the bad behavior you can add one of these as a hack fix, but the underlying problem remains to catch the next unsuspecting user.

I'd rather remove the landmine than keep bypassing it, and putting up a sign saying "don't use auto", or "always use const", just marks the minefield, it doesn't remove it.

How can I make my library immune to this gotcha? (Without changing the client code!)

  • First preference would be the code works as written - assert(allTheData[1] == false) passes
    • A way to define the decay type of the proxy when it's written to auto?. So decltype(boolValue) is bool?
    • An implicit conversion operator that takes precedence over copying?
    • Any other way to make this pass without changing the code snippet above?
  • Second preference is there a way to make writing a proxy to a variable a compile error?
    • I'm declaring copy and move constructors as delete, and move and copy assignment operators as delete. Still compiles.
    • Is there anyway to declare a class as unable to become a lvalue?
  • Is there anything in proposed c++ future standards that will fix this?

Also an issue is code like:

std::vector<bool> ReadFlags();
... later ...
auto databaseIsLockedFlag = ReadFlags()[FLAG_DB_LOCKED];
if (databaseIsLockedFlag) <-- Crash here. Proxy has outlived temporary vector.

I'm only using vector here as it's a really simple example of the problem. This isn't a bug with vector, this is a bug with the proxy type pattern, of which vector is an example to show the problem.

Strangely enough MSVC's Intellisense engine sometimes reports copying a no-move-no-copy proxy type as a compile error, but then compiles it fine anyway:

enter image description here
It'd be really nice if this intellisense compile error was a real compile error. Sigh

Drew Dormann
  • 59,987
  • 13
  • 123
  • 180
Ash
  • 1,956
  • 1
  • 14
  • 15
  • note that `std::vector` can be considered broken, a common "fix" is to not use it – 463035818_is_not_an_ai Apr 03 '21 at 11:11
  • 1
    Can you show us actual representative code? Deleting the copy constructor should generally work. It sounds like in your case we need to further constrain what the constructors accept – AndyG Apr 03 '21 at 11:12
  • 4
    @largest_prime_is_463035818 Yes I'm aware of this. I pointed out "I'm only using vector here as it's a really simple example of the problem". I just needed a simple example of the problem to give a minimum reproducible example and is only used so I can show the problem in a reproducible case in 5 lines rather than 100. – Ash Apr 03 '21 at 11:15
  • 1
    @Ash I am just saying that `std::vector` was known to be an issue already before `auto`, and the most common fix afaik is to just not use it. To find a better fix for your case it would be better if you show your case, not just describe it in words. Can you post a [mcve] ? – 463035818_is_not_an_ai Apr 03 '21 at 11:17
  • 4
    @AndyG Complete standalone example that doesn't use std::vector to simplify the problem at https://godbolt.org/z/YzP57vnbf – Ash Apr 03 '21 at 11:36
  • 30
    @largest_prime_is_463035818 the OP already stated his/hers use case: proxy classes in math library. That is a common pattern in math libraries dealing with operations on large data like vectors and matrices. So please don't get hung up on the `std::vector` as this question has real value and really isn't about `std::vector` at all. – bolov Apr 03 '21 at 12:28
  • You indicate that you are looking here for a way to stop the users from shooting themselves in the foot, but one lesson I’m taking from this is that `auto` should be considered harmful. If I added to the scope, `using container_t = std::vector; using element_t = container_t::value_type;`, the line `element_t boolValue = allTheData[1];` would have the same maintainability if I wanted to change the data structure, and really would create a local copy like I wanted. – Davislor Apr 05 '21 at 01:53
  • Alternatively, this wouldn’t occur if I used static single assignments. It’s trivial to do that for `boolValue`, but I’m more likely to need to update the `vector` in place. – Davislor Apr 05 '21 at 01:57
  • 1
    offtopic, but similar problem is with fake value types, e.g. std::span https://www.godbolt.org/z/r3Yqfoao9 , not just weirdo vector – NoSenseEtAl Apr 05 '21 at 14:19
  • 4
    The root problem is, that `auto` is evil: It hides important facts about the variable that is being declared. In this case, that is the fact that `boolValue` is a reference, but other bits like the actual type of the variable that immensely helps in reasoning about the code are hidden as well, possibly forcing a programmer to look into several different files in order to deduce a single variable type. There is no advantage in a polished appearance of the code if the result is that the code is harder to understand. As such, the only real fix is to avoid using `auto` when not necessary. – cmaster - reinstate monica Apr 06 '21 at 14:34

4 Answers4

71

Reduce the damage by adding "&&" to the end of the proxy class's operator=

(And operator +=, -=, etc.)

Took me a lot of experimenting but I eventually found a way to mitigate the most common case of the problem, this tightens it so you can still copy the proxy, but once you've copied it to a stack variable, you can't modify it and inadvertently corrupt the source container.

#include <cstdio>
#include <utility>

auto someComplexMethod()
{
  struct s
  {
    void operator=(int A)&& {std::printf("Setting A to %i", A);}
  };
  return s();
}

int main()
{
  someComplexMethod() = 4; // Compiles. Yay

  auto b = someComplexMethod(); 
  // Unfortunately that still compiles, and it's still taking a 
  // copy of the proxy, but no damage is done yet.

  b = 5; 
  // That doesn't compile. Error given is: 
  //   No overload for '='  note: candidate function not viable: 
  //   expects an rvalue for object argument

  std::move(b) = 6; 
  // That compiles, but is basically casting around the 
  // protections, aka shooting yourself in the foot.
}
Ash
  • 1,956
  • 1
  • 14
  • 15
  • 3
    What is the name of this feature? I have never seen it used and would like to read about it. – oarfish Apr 04 '21 at 15:53
  • 4
    @oarfish Such member functions are said to be *rvalue-ref-qualified*. – HolyBlackCat Apr 04 '21 at 16:00
  • 1
    See also [What is “rvalue reference for *this”?](https://stackoverflow.com/q/8610571/7509065) – Joseph Sible-Reinstate Monica Apr 04 '21 at 17:32
  • 5
    This looks like it only solves the problem in one direction. If vector used your fix, then the code in your question would be safe, but `vector v = {false}; auto b = v[0]; v[0] = true; assert(!b);` would blow up still. – Joseph Sible-Reinstate Monica Apr 04 '21 at 17:52
  • 3
    @Jose All operations on `b` need to be `&&` qualified. Then `assert(!b)` would fail to compile. – Yakk - Adam Nevraumont Apr 04 '21 at 18:34
  • Even though it looks clever, this is a bad idea as it will lure you into a false sense of security. There are many ways the mutability can escape. For instance, `auto someFunc() { return someComplexMethod(); }` [**does** allow](https://godbolt.org/z/fGW7T9xor) `someFunc() = false`. – spectras Apr 04 '21 at 23:11
  • 3
    @spectras: But no one's going to write `someFunc() = false` unless they know it's a proxy and want the behavior they're going to get. Going back to the `vector` example, the analogue of `someFunc() = false` in the `vector` example is `v[0] = false`, which we *want* to allow. – user2357112 Apr 05 '21 at 06:39
  • @spectras That example is pretty obvious that writing is the intended behavior. When the confusion between a proxy and a value is a problem and you dont know what type something is, assigning to it makes it pretty clear you want side effects. Is there any better examples you can think of? – Ash Apr 05 '21 at 06:46
  • The point is not on how likely it is one writes that, it is on the fact that this is possible, which is a major hole in the abstraction. In a simple showcase example like this, spotting the issue is easy. How sure you are to spot it once your type is used with some templated function you did not write yourself? And that some sfinae, some `decltype(auto)` or other form of type reflection in there won't make your operator= usable and silently call it, no warnings issued? – spectras Apr 05 '21 at 09:24
  • @spectras: The whole point of the proxy is to make assignment possible. If you don't want that behavior, it's straightforward to delete the assignment operator entirely, or just not use a proxy, but who wants a `vector` where `v[0] = false` is banned or doesn't update `v`? – user2357112 Apr 05 '21 at 12:04
  • 1
    @user2357112supportsMonica> you should examine my example more thoroughly. The point of the proxy is to make assignment possible where it is used, not that it gets forwarded and that you leak the possibility to mutate internal values 3 layers up the call stack. – spectras Apr 05 '21 at 14:27
  • +1, for that matter, making all public members `&&`-qualified (or even `const&&`-qualified) could help. This is the solution I am settling after C++17 prevented me for making `auto a = proxy();` a syntax error. https://godbolt.org/z/4dn16f16o – alfC Jun 10 '23 at 20:29
61

Yes, this is indeed a problem. Afaik there is no solution to this in current C++ (C++20 at the time of writing) other than to change the code at the calling site which is not ideal.

There is the proposal P0672R0 Implicit Evaluation of “auto” Variables (from 2017) which tries to deal with this exact problem. It uses as example proxy classes in math libraries just like your case and gives example with std::vector<bool> just like you. And it gives more examples of problems that arrive from this pattern.

The paper proposes 3 solutions to this, all implemented in language:

  • operator notation:

    class product_expr
    {
        matrix operator auto() { ... }
    };
    
  • using declaration:

    class product_expr
    {
        using auto = matrix;
    };
    
  • specialization of decay:

    make auto x= expr be defined as typename std::decay<decltype(expr)>::type x=expr; and then use used can specialize std::decay

The discussion at the standard committee meetings strongly favored the using declaration solution. However I wasn't able to find any more updates on this paper so I personally don't see this paper or something similar being implemented in the language in the near future.

So, unfortunately, for now your only solution is to educate your users on the proxy classes your library uses.

bolov
  • 72,283
  • 15
  • 145
  • 224
36

I have an obscure idea, not sure how practical it is. It doesn't override what auto deduces to (which seems impossible), but merely causes copying the proxy to a variable ill-formed.

  • Make the proxy non-copyable.

  • That alone doesn't stop you from saving the proxy to an auto variable, due to mandatory RVO. To counteract it, you return the proxy by reference instead.

  • To avoid getting a dangling reference, you construct the proxy in a default function argument, which have the same lifetime as regular arguments - until the end of a full expression.

  • User could still save a reference to the proxy. To make this kind of misuse harder, you return an rvalue-reference, and &&-qualify all member functions.

  • This prevents any interaction with the dangling proxy reference, unless you std::move it. This should be obscure enough to stop your users, but if not, you'll have to rely on some sanitizer or set a (volatile?) flag in the destructor of the proxy, and check it each time you access it (which is UB, but should be reliable enough).

Example:

namespace impl
{
    class Proxy
    {
        Proxy() {}
      public:
        static Proxy construct() {return {};}
        Proxy(const Proxy &) = delete;
        Proxy &operator=(const Proxy &) = delete;
        int *ptr = nullptr;
        int operator=(int value) && {return *ptr = value;}
    };
}

impl::Proxy &&make_proxy(int &target, impl::Proxy &&proxy = impl::Proxy::construct())
{
    proxy.ptr = &target;
    return std::move(proxy);
}

Then:

int x = 0;
make_proxy(x) = 1; // Works.
auto a = make_proxy(x); // error: call to deleted constructor of 'impl::Proxy'
auto &b = make_proxy(x); // error: non-const lvalue reference to type 'impl::Proxy' cannot bind to a temporary of type 'impl::Proxy'
const auto &c = make_proxy(x); // Compiles, is a dangling reference. BUT!
c = 2; // error: no viable overloaded '='
auto &&d = make_proxy(x); // Compiles, is a dangling reference.
d = 3; // error: no viable overloaded '='
std::move(d) = 2; // Compiles, causes UB. Needs a runtime check.

This is harder to pull off with overloaded operators, which (except ()) can't have default arguments. But still doable:

namespace impl
{
    struct Index
    {
        int value = 0;
        Proxy &&proxy;
        Index(int value, Proxy &&proxy = Proxy::construct()) : value(value), proxy(std::move(proxy)) {}
        Index(const Index &) = delete;
        Index &operator=(const Index &) = delete;
    };
}

struct B
{
    int x = 0;
    impl::Proxy &&operator[](impl::Index index)
    {
        index.proxy.ptr = &x;
        return std::move(index.proxy);
    }
};

The only downside is that since at most one user-defined implicit conversion is allowed for any argument, this operator[] will only work with int arguments, and not with classes with operator int.

HolyBlackCat
  • 78,603
  • 9
  • 131
  • 207
0

What about this:

#include <array>

struct Container;
struct Proxy;
struct Handle { 
    Container* my;
    int myIndex;
};


struct Proxy
{
    void operator=(int Incomming) const &&;
    operator const int() const;
   
private:
    Proxy(const Proxy& o) = delete; 
    Proxy(Proxy&& o) = delete;

    const Handle  h;

    Proxy(const Handle & _h) : h(_h) {}
    friend struct Container;

    void operator=(Proxy&&) = delete;
    void operator=(const Proxy&) = delete;
};


struct Container
{
    Proxy operator[](int I)
    {
        return Handle{this, I};
    }

    private:
    std::array<int, 16> impl;
    friend struct Proxy;
};



void Proxy::operator=(int Incomming) const &&
{
    h.my->impl[h.myIndex] = Incomming;
}

Proxy::operator const int() const
{
   return h.my->impl[h.myIndex];
}



int main()
{
    Container c;

    c[4] = 2;
    auto d = c[4];
    d = 1; // ERROR: Doesn't compile, since inaccessible because d is a Proxy not a Proxy &&

    return c[4]; // Returns 2
}

Demo here

It's much better than @Ash answer since it doesn't require using a method to access the proxy, you'll use it like a usual object (see main above). This will prevent taking a proxy and manipulating later on, you must manipulate it while it's still a rvalue reference.

I've added a Handle class so the proxy can't be constructed from outside the Container.

xryl669
  • 3,376
  • 24
  • 47