Skip to content

Update wording on __attribute__((malloc)) #937

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Update wording on __attribute__((malloc)) #937

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions docs/Compiler-Hardening-Guides/Compiler-Annotations-for-C-and-C++.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ Table 1: Recommended attributes
| Attribute | Supported since | Type | Description |
|:------------------------------------------------------------------------------------------ |:---------------------------:|:----------------------------:|:------------------------------------------------------------------------------------------------- |
| `malloc` | GCC 2.95.3<br/>Clang 13.0.0 | Function | Mark custom allocation functions that return non-aliased (possibly NULL) pointers. |
| `malloc (`_`deallocator`_`)` | GCC 11.0.0<br/>Clang 21.0.0 | Function | Associates _`deallocator`_ as the valid deallocator for the storage allocated by marked function. |
| `malloc (`_`deallocator`_`)` | GCC 11.0.0 | Function | Associates _`deallocator`_ as the valid deallocator for the storage allocated by marked function. |
| `ownership_returns(`_`allocation-type`_`)` | Clang 20.1.0 | Function | Associate pointers returned by custom allocation function with _`allocation-type`_ . |
| `ownership_takes(`_`allocation-type`_`,` _`ptr-index`_`)` | Clang 20.1.0 | Function | Mark function as valid deallocator for _`allocation-type`_. |
| `ownership_holds(`_`allocation-type`_`,` _`ptr-index`_`)` | Clang 20.1.0 | Function | Mark function taking responsibility of deallocation for _`allocation-type`_. |
Expand All @@ -56,7 +56,7 @@ Attributes influence not only diagnostics generated by the compiler but also the
| Attribute | Supported since | Type | Description |
|:--------------------------------------------------------------------------------------------|:---------------------------:|:----------------------------:|:------------------------------------------------------------------------------------------------- |
| <span id="malloc">`malloc`</span> | GCC 2.95.3<br/>Clang 13.0.0 | Function | Mark custom allocation functions that return non-aliased (possibly NULL) pointers. |
| <span id="malloc (dealloc)">`malloc (`_`deallocator`_`)`</span> | GCC 11.0.0<br/>Clang 21.0.0 | Function | Associates _`deallocator`_ as the valid deallocator for the storage allocated by marked function. |
| <span id="malloc (dealloc)">`malloc (`_`deallocator`_`)`</span> | GCC 11.0.0 | Function | Associates _`deallocator`_ as the valid deallocator for the storage allocated by marked function. |
Copy link
Contributor

@thesamesam thesamesam Jul 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not having Clang here at all is confusing as it implies you can't safely use the attribute, no? You can safely use it, just it doesn't do anything.

Copy link
Author

@Quuxplusone Quuxplusone Jul 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It produces a warning (or an error with -Werror), which in practice should break your build. If you're not compiling with -Werror — or if you're letting warnings happen and not fixing them — I think that's vastly more of a departure from "security best practices" than omitting to use some attribute.

I guess one criterion to answer "Does Clang support this attribute?" is to see whether a cross-platform build system like CMake or Meson believes that Clang supports it. I grepped github for __attribute__((malloc, and one of the top hits was this commit to Meson: OpenRC/openrc@17de4e5 —

# Meson's has_function_attribute doesn't know about GCC's extended
# version (with arguments), so we have to build a test program instead.
malloc_attribute_test = '''#include<stdlib.h>
__attribute__ ((malloc (free, 1)))
int func() { return 0; }
'''
if cc.compiles(malloc_attribute_test, name : 'malloc attribute with arguments')
  add_project_arguments('-DHAVE_MALLOC_EXTENDED_ATTRIBUTE', language: 'c')
endif

When you feed this to Meson on a Clang system, does it indeed set -DHAVE_MALLOC_EXTENDED_ATTRIBUTE=1? If it does, then I'll (grudgingly :)) accept that in practice Clang trunk is regarded as supporting the extended malloc attribute, even though the Clang maintainers themselves say it's not supported yet. Sadly I don't have a Meson system to test this on myself.

Copy link
Contributor

@thomasnyman thomasnyman Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can understand both viewpoints on this. I also see that listing Clang under the "Supported since" column give the wrong impression especially given the current behavior that issues a warning. What I might suggest is to add a note Clang in the description to the effect of "Clang trunk has this attribute, but lacks the corresponding diagnostic (resulting in -Wignored-attributes warning on use)." which we'll update to say Clang 21.0.0 once it is released.

Copy link
Contributor

@thesamesam thesamesam Jul 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this commit to Meson

Right, my commit to a project using Meson (not Meson itself).

Sadly I don't have a Meson system to test this on myself.

I think we've maybe reached an impasse as I don't have a Clang trunk system to test it on ;)

If I extract the command line used and the program from meson-log.txt on godbolt, I see this:

<source>:2:17: warning: 'malloc' attribute only applies to return values that are pointers [-Wignored-attributes]
    2 | __attribute__ ((malloc (free, 1)))
      |                 ^
    3 | int func() { return 0; }
      | ~~~
1 warning generated.
Compiler returned: 0

so I think yes, it'll pass?

Copy link

@eli-schwartz eli-schwartz Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It produces a warning (or an error with -Werror), which in practice should break your build. If you're not compiling with -Werror — or if you're letting warnings happen and not fixing them — I think that's vastly more of a departure from "security best practices" than omitting to use some attribute.

The idea that -Werror must be used and fail the build on any form of error is an admirable one, but it necessarily has a rough interaction with the fact that C/C++ compilers don't have a structured way to distinguish between coding style (opinionated) warnings and semantic warnings.

In general, it is most emphatically NOT a code smell or a violation of security best practices to react to warnings -- or even -Werrors -- by determining the warning is too much of "opinionated lint" an silencing it via -Wno-ignored-attributes or similar.

So, eminently reasonable to safely use it everywhere?

Copy link
Author

@Quuxplusone Quuxplusone Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh. I think OSSF and I just have different opinions about the meanings of the words "safe" and "best practices." Like, if a feature is not supported on one of my targeted compilers, I'd use a macro to avoid it; e.g.

#if MY_COMPILER_GCC
 #define MY_ATTR_MALLOC __attribute__((malloc, malloc(free)))
#else
 #define MY_ATTR_MALLOC
#endif

[...]

int *func() MY_ATTR_MALLOC { return malloc(sizeof(int)); }

Then I'd "treat warnings as errors" and make sure my code compiled cleanly on all of my targeted compilers.

It sounds like the OSSF way is to put the attribute unconditionally, and then use -Wno-ignored-attributes in the project file (because by definition the project cannot be made clean w.r.t. that warning anymore). In my style, that would be frowned on; we'd prefer to fix the warning rather than disable it for the whole team.

Anyway, as I'm not part of OSSF, I'll abandon this PR.

| <span id="malloc (dealloc, ptr-index)">`malloc (`_`deallocator`_`,` _`ptr-index`_`)`</span> | GCC 11.0.0 | Function | Same as above but also denotes the positional argument here the pointer must be passed. |
| <span id="ownership_returns">`ownership_returns(`_`allocation-type`_`)`</span> | Clang 20.1.0 | Function | Associate pointers returned by custom allocation function with _`allocation-type`_ . |
| <span id="ownership_takes">`ownership_takes(`_`allocation-type`_`,` _`ptr-index`_`)`</span> | Clang 20.1.0 | Function | Mark function as valid deallocator for _`allocation-type`_. |
Expand All @@ -75,7 +75,7 @@ In GCC, the `malloc (`_`deallocator`_`)` and `malloc (`_`deallocator`_`,` _`ptr-
- **Memory leaks** (`-Wanalyzer-malloc-leak`) if if there is an execution path in which the result of an allocation call goes out of scope without being passed to the deallocation function.
- **Invalid free** (`-Wanalyzer-free-of-non-heap`) if a deallocation function is used on a global or on-stack variable.

Clang supports both forms of the `malloc` attribute but does not yet implement the `-Wmismatched-dealloc` and `-Wmismatched-new-delete` warnings. Instead, Clang provides the `ownership_returns`, `ownership_takes`, and `ownership_holds` attributes[^clang-ownership]: that interact with the Clang static analyzer[^clang-checkers].
Clang's parser recognizes both forms of the `malloc` attribute, but Clang does not yet implement the `-Wuse-after-free`, `-Wmismatched-dealloc`, and `-Wmismatched-new-delete` warnings that would use that information. Instead, Clang provides the `ownership_returns`, `ownership_takes`, and `ownership_holds` attributes[^clang-ownership]: that interact with the Clang static analyzer[^clang-checkers].

In Clang, the `ownership_returns(`_`allocation-type`_`)` associates the pointer returned by the marked function with an _`allocation-type`_. Here, _`allocation-type`_ is any string which will subsequently be used to detect mismatched allocations in cases where the pointer is passed to a deallocator marked with another _`allocation-type`_. The _`allocation-type`_ `malloc` has a special meaning and causes the Clang static analyzer to treat the associated pointer as though the allocated storage would have been allocated using the standard `malloc()` function, and can subsequently be safely deallocated with the standard `free()` function.

Expand All @@ -84,8 +84,8 @@ The Clang `ownership_takes(`_`allocation-type`_`,` _`ptr-index`_`)` attribute ma
Using the the `ownership_returns`, `ownership_takes`, and `ownership_holds` attributes allows the Clang static analyzer to catch:

- **Mismatched deallocation** (`unix.MismatchedDeallocator`) if there is an execution path in which the result of an allocation call of type _`allocation-type`_ is passed to a function annotated with `ownership_takes` or `ownership_holds` with a different allocation type.
- **Double free** (`unix.Malloc`, `cplusplis.NewDelete`) if there is an execution path in which a value is passed more than once to a function annotated with `ownership_takes` or `ownership_holds`.
- **Use-after-free** (`unix.Malloc`, `cplusplis.NewDelete`) if there is an execution path in which the memory passed by pointer to a function annotated with `ownership_takes` is used after the call. Using memory passed to a function annotated with `ownership_holds` is considered valid.
- **Double free** (`unix.Malloc`, `cplusplus.NewDelete`) if there is an execution path in which a value is passed more than once to a function annotated with `ownership_takes` or `ownership_holds`.
- **Use-after-free** (`unix.Malloc`, `cplusplus.NewDelete`) if there is an execution path in which the memory passed by pointer to a function annotated with `ownership_takes` is used after the call. Using memory passed to a function annotated with `ownership_holds` is considered valid.
- **Memory leaks** (`unix.Malloc`, `cplusplus.NewDeleteLeaks`) if if there is an execution path in which the result of an allocation call goes out of scope without being passed to a function annotated with `ownership_takes` or `ownership_holds`.
- **Dubious `malloc()` arguments involving `sizeof`** (`unix.MallocSizeof`) if the size of the pointer type the returned pointer does not match the size indicated by `sizeof` expression passed as argument to the allocation function.
- **Potentially attacker controlled `size` parameters to allocation functions** (`optin.taint.TaintedAlloc`) if the `size` parameter originates from a tainted source and the analyzer cannot prove that the size parameter is within reasonable bounds (`<= SIZE_MAX/4`).
Expand All @@ -101,12 +101,12 @@ void my_free(void *ptr);
void *my_malloc(size_t size) __attribute__ ((malloc, malloc (my_free, 1)));
~~~

Note that to benefit both from the associated optimizations and improved detection of memory errors functions should be marked with _both_ the form of the attribute without arguments and the form of the attribute with one or two arguments. [[Extended example at Compiler Explorer](https://godbolt.org/z/bc97ahbnd)]
Note that to benefit from _both_ the no-alias optimization and the improved detection of memory errors, functions should be marked with _both_ the zero-argument `malloc` attribute and the form of the attribute with one or two arguments. Note that an allocation function whose return value may alias another object (for example as a non-null "flag" or "sentinel" value) must not be marked with the zero-argument `malloc` attribute, but can still safely be marked with the one- or two-argument form. [[Extended example at Compiler Explorer](https://godbolt.org/z/PheGfrvdc)]

Clang `ownership_returns`, `ownership_takes`, and `ownership_holds`:

~~~c
// Denotes that my_malloc will return with a pointer to storage of labeled as "my_allocation" .
// Denotes that my_malloc will return a pointer to storage with the label "my_allocation" .
void *my_malloc(size_t size) __attribute((malloc, ownership_returns(my_allocation)));

// Denotes that my_free will deallocate storage pointed to by ptr that has been labeled "my_allocation".
Expand Down