isocpp / CppCoreGuidelines

The C++ Core Guidelines are a set of tried-and-true guidelines, rules, and best practices about coding in C++
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines
Other
42.53k stars 5.43k forks source link

Discourage implicit casting from/to void pointer #2207

Open dfrvfmst opened 3 months ago

dfrvfmst commented 3 months ago

In both ES.48: Avoid casts and Pro.safety: Type-safety profile I see no mention regarding the danger of (implicitly or explicitly) casting from/to void pointer. Since it strips type info away, the caller can do abysmal things to the pointer type. Example:

void *suppress_warning(void *ptr)
{
    return ptr;
}

int main()
{
    gsl::owner<char *> a = new char{};
    volatile auto *p = static_cast<double *>(suppress_warning(a));
    *p = 3; // <--- Detected by ASan: heap-buffer-overflow (WRITE of size 8)
    delete a;
}

This would be equivalent of a single reinterpret_cast / C-style cast, except it's not covered by C++ Core Guidelines.

Currently, clang-tidy doesn't generate a warning for it. If this feels obvious enough to catch by just looking at the code block, it becomes more subtle when the void pointer is used to hold a resource that is returned from a C-style API.

Example of returning resource with void pointer:

// The API documents that it returns an owning resource of
// either char or double based on the provided argument(s).
gsl::owner<void *> get(bool flag)
{
    if (flag) {
        return new char{}; // <--- This is the returned value
    } else {
        return new double{};
    }
}

int main()
{
    gsl::owner<volatile double *> p = static_cast<double *>(get(true));
    *p = 3; // <--- Detected by ASan: heap-buffer-overflow (WRITE of size 8)
    delete p;
}

Source: https://github.com/trailofbits/clang-cfi-showcase/blob/23405d0e5b5aa4f1e7c456628ec3e0a09be32694/cfi_unrelated_cast.cpp#L34

On a side note, this is also the only C++ example in that project which clang-tidy failed to provide any meaningful warning and had to resort to runtime checks (ASan / CFI).


The issue seems to be only fixable from the interface developer side, there isn't much that can be done from the user side apart from outright disallowing casts from void pointer (which would sadly make those unsafe API unusable).

For C++ codes, instead of void pointer, the interface developer can use std::variant for returning different types of pointer:

auto get(bool flag) -> std::variant<gsl::owner<char *>, gsl::owner<double *>>
{
    if (flag) {
        return new char{};
    } else {
        return new double{};
    }
}

int main()
{
    gsl::owner<volatile double *> p = std::get<gsl::owner<double *>>(get(true));
    *p = 3;
    delete p;
}

This results in a std::bad_variant_access exception and therefore blocks the user from misusing the API.

iglesias commented 2 months ago

In both ES.48: Avoid casts and Pro.safety: Type-safety profile I see no mention regarding the danger of (implicitly or explicitly) casting from/to void pointer.

I think that maybe those focus on casting and the types of casts, without entering in what are the types being casted.

Usage of void* altogether is discussed in Per.10, T.3's enforcement ("Flag uses of void* and casts outside low-level implementation code"), CPL.1 ("The rules for implicit casting to and from void* in C are subtle and unenforced.").