False-positive for lifetimebound functions returning a reference to a pointer/view

[usx95]

This came up while running the new lifetime analysis in Google. We have many false-positives involving absl::StatusOr accessor methods which mark the implicit this object parameter as lifetimebound. This gives false-positives in cases where the StatusOr doesn't own the underlying data (that is when the type is a pointer or view type).

Reproducer: https://godbolt.org/z/oMhdTTv78

#include <iostream>
#include <string>
#include <string_view>

template <class T>
struct StatusOr {
    ~StatusOr() {}
    const T& value() const& [[clang::lifetimebound]] { return data; }

   private:
    T data;
};

StatusOr<std::string_view> getViewOr();
StatusOr<std::string> getStringOr();
StatusOr<std::string*> getPointerOr();

void foo() {
    std::string_view view;
    {
        StatusOr<std::string_view> view_or = getViewOr();
        view = view_or.value();
    }
    std::cout << view;  // error: a use-after-free. Bad!
}

void bar() {
    std::string* pointer;
    {
        StatusOr<std::string*> pointer_or = getPointerOr();
        pointer = pointer_or.value();
    }
    std::cout << *pointer;  // error: a use-after-free. Bad!
}

void foobar() {
    std::string_view view;
    {
        StatusOr<std::string> string_or = getStringOr();
        view = string_or.value();
    }
    std::cout << view;  // error: a use-after-free. Good!
}

See https://godbolt.org/z/EvdTjoq38 for original issue with absl::StatusOr src.

[llvmbot]

@llvm/issue-subscribers-clang-temporal-safety

Author: Utkarsh Saxena (usx95)

This came up while running the new lifetime analysis in Google. We have many false-positives involving absl::StatusOr accessor methods which mark the implicit this object parameter as lifetimebound. This gives false-positives in cases where the StatusOr doesn't own the underlying data (that is when the type is a pointer or view type).

Reproducer: https://godbolt.org/z/oMhdTTv78

#include <iostream>
#include <string>
#include <string_view>

template <class T>
struct StatusOr {
    ~StatusOr() {}
    const T& value() const& [[clang::lifetimebound]] { return data; }

   private:
    T data;
};

StatusOr<std::string_view> getViewOr();
StatusOr<std::string> getStringOr();
StatusOr<std::string*> getPointerOr();

void foo() {
    std::string_view view;
    {
        StatusOr<std::string_view> view_or = getViewOr();
        view = view_or.value();
    }
    std::cout << view;  // error: a use-after-free. Bad!
}

void bar() {
    std::string* pointer;
    {
        StatusOr<std::string*> pointer_or = getPointerOr();
        pointer = pointer_or.value();
    }
    std::cout << *pointer;  // error: a use-after-free. Bad!
}

void foobar() {
    std::string_view view;
    {
        StatusOr<std::string> string_or = getStringOr();
        view = string_or.value();
    }
    std::cout << view;  // error: a use-after-free. Good!
}

See https://godbolt.org/z/EvdTjoq38 for original issue with absl::StatusOr src.

[usx95]

The core of the problem is that the analysis currently does not distinguish between the lifetime of a reference to a view and the lifetime of the view itself. This leads to false positives when a function returns a reference to a pointer or view, and the lifetimebound attribute is applied to the function.

We have been discussing about a principled way to address this by introducing the concept of multiple lifetimes (aka "origins") for a type. I think it would make sense to introduce some form of multi-origin support (even if it is in the form of only a pair of origins) right away instead of waiting to address more complicated cases later and resorting to hacky heuristics immediately. (cc: @Xazax-hun as this might be relevant to our recent discussions)

More details... For example, a type like const std::string_view& would have two origins:

• One for the reference itself.
• One for the value of the string_view (the pointee).

We would represent this as const std::string_view^2 &^1.

With this model, the [[clang::lifetimebound]] attribute on const T& StatusOr<T>::value() would only apply to the outer origin (^1), which is the reference. The inner origin (^2) would be treated as opaque and unconstrained.

In the expression view = view_or.value();, the LValueToRValue conversion of the call to value() would drop the outer origin which represents the outer LValue (^1). Only the inner origin (^2) would be associated with the view variable. Since ^2 has no lifetime constraints (is opaque), the analysis would determine that there is not information to conclude a use-after-free.

This approach also correctly handles the case where StatusOr owns the underlying data. For example, with StatusOr<std::string>, value() returns a const std::string&. This type has only a single origin (const std::string&^1).
The user-defined convertor converts the lvalued std::string to an std::string_view^2 and this also adds a constraint ^2: ^1. At this point, ^2 is no more opaque and thus it must not outlive the StatusOr object.

This solution is general and can be extended to handle pointers to pointers, containers of pointers, and other complex types by associating multiple origins with them. But the immediate plan is to expand our model to types with two origins (reference/pointer to view/pointer).

[Xazax-hun]

This sounds great! This is exactly what I had in mind.