Annotate alloc/free open/close functions with compiler attributes by MisterDA · Pull Request #13457 · ocaml/ocaml

MisterDA · 2024-09-19T18:09:35Z

This helps the C compiler optimize code, and helps static analysis by detecting potential mismatches in alloc/free pairs.

malloc

The malloc attribute indicates that the function acts like a system memory allocation function, returning a pointer to allocated storage disjoint from the storage for any other object accessible to the caller.

https://clang.llvm.org/docs/AttributeReference.html#malloc

Associating a function with a deallocator helps detect calls to mismatched allocation and deallocation functions and diagnose them under the control of options such as -Wmismatched-dealloc. It also makes it possible to diagnose attempts to deallocate objects that were not allocated dynamically, by -Wfree-nonheap-object. To indicate that an allocation function both satisifies the nonaliasing property and has a deallocator associated with it, both the plain form of the attribute and the one with the deallocator argument must be used.

The warnings guarded by -fanalyzer respect allocation and deallocation pairs marked with the malloc attribute.

https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-malloc-function-attribute

Note that the malloc attribute can only be applied to functions returning pointers. The OCaml value type is a typedef to an integer type, and the C compiler will refuse applying the attribute to a function returning an OCaml value (one could want to check if custom values are finalized).
nodiscard / warn_unused_result

Prevent memory leaks by warning if the result of an allocation is ignored.

https://clang.llvm.org/docs/AttributeReference.html#nodiscard-warn-unused-result
alloc_align

GCC uses this information to improve pointer alignment analysis.

https://clang.llvm.org/docs/AttributeReference.html#alloc-align
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005falign-function-attribute
alloc_size

GCC uses this information to improve the results of __builtin_object_size.

https://clang.llvm.org/docs/AttributeReference.html#alloc-size
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute
returns_nonnull

lets the compiler optimize callers based on the knowledge that the return value will never be null.

The analyzer considers the possibility that an allocation function could fail and return null. […] If the allocator always returns non-null, use __attribute__ ((returns_nonnull)) to suppress these warnings.

https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-returns_005fnonnull-function-attribute

Invoked on the following test program, GCC 14.2.1 reports:

#include <caml/memory.h> /* with this patch */
#include <stdio.h>

void test_alloc_wrong_free(void)
{
  void *data = caml_stat_alloc(42);
  free(data);
}

void test_discard_alloc_result(void)
{
  caml_stat_alloc(42);
}

void test_alloc_noexec_null(void)
{
  void *data = caml_stat_alloc(42);
  printf("hello, world!");
}

opam exec -- gcc $(ocamlopt -config-var native_c_flags) \
                 -I $(ocamlopt -config-var standard_library_default) \
                 $(ocamlopt -config-var native_ldflags) -c \
                 -Wall -fanalyzer ./test.c

./test.c: In function ‘test_alloc_noexec_null’:
./test.c:17:9: warning: unused variable ‘data’ [-Wunused-variable]
   17 |   void *data = caml_stat_alloc(42);
      |         ^~~~
./test.c: In function ‘test_discard_alloc_result’:
./test.c:12:3: warning: ignoring return value of ‘caml_stat_alloc’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
   12 |   caml_stat_alloc(42);
      |   ^~~~~~~~~~~~~~~~~~~
./test.c: In function ‘test_alloc_wrong_free’:
./test.c:7:3: warning: ‘data’ should have been deallocated with ‘caml_stat_free’ but was deallocated with ‘free’ [CWE-762] [-Wanalyzer-mismatching-deallocation]
    7 |   free(data);
      |   ^~~~~~~~~~
  ‘test_alloc_wrong_free’: events 1-2
    |
    |    6 |   void *data = caml_stat_alloc(42);
    |      |                ^~~~~~~~~~~~~~~~~~~
    |      |                |
    |      |                (1) allocated here
    |    7 |   free(data);
    |      |   ~~~~~~~~~~    
    |      |   |
    |      |   (2) deallocated with ‘free’ here
    |
./test.c: In function ‘test_alloc_noexec_null’:
./test.c:19:1: warning: leak of ‘data’ [CWE-401] [-Wanalyzer-malloc-leak]
   19 | }
      | ^
  ‘test_alloc_noexec_null’: events 1-2
    |
    |   17 |   void *data = caml_stat_alloc(42);
    |      |                ^~~~~~~~~~~~~~~~~~~
    |      |                |
    |      |                (1) allocated here
    |   18 |   printf("hello, world!");
    |   19 | }
    |      | ~               
    |      | |
    |      | (2) ‘data’ leaks here; was allocated at (1)
    |
./test.c: In function ‘test_alloc_wrong_free’:
./test.c:7:3: warning: ‘free’ called on pointer returned from a mismatched allocation function [-Wmismatched-dealloc]
    7 |   free(data);
      |   ^~~~~~~~~~
./test.c:6:16: note: returned from ‘caml_stat_alloc’
    6 |   void *data = caml_stat_alloc(42);
      |                ^~~~~~~~~~~~~~~~~~~

Simply using -Wall turns on more checks. The -fanalyzer warning has both false positive and false negatives and I'm not turning it on on the codebase. I believe this code may help users writing C FFI code for OCaml. No warnings have been raised on the current trunk or on Lwt. Let me know if there are packages containing C code that could be tested.

glibc has annotated functions in malloc/malloc.h, but as we're keeping track of C heap allocations, the compiler cannot propagate the annotations to the call sites of caml_stat_* functions.

I don't have insights on whether code gen has improved or worsened.

I tried to keep the macro definitions as condensed as possible, let me know if you'd prefer an alternate definition.

gasche · 2024-09-19T19:15:20Z

I like the idea. I looked at the document to figure out what the numbers in alloc_size(1, 2) mean, they refer to the position of the parameter giving size information (if there are several, it's the product of those parameters).

Style nitpick: I'm not fond of the fact that sometimes the user is passing some constant numbers by parameter (the ptr_index parameters), and sometimes they are hidden in the macro definition. I think that they should never be passed by the user (always hidden in the macro), or always passed by the user (so it would be CAMLaligned_alloc(caml_stat_free, 1, 2) for example).

MisterDA · 2024-09-20T13:09:30Z

Do we have other pairs of allocators/deallocators functions that could benefit from these annotations? If not, I'm tending towards making all the parameters hidden. If there are, or if we could have some use for them in the future, explicit parameters (as the expense of adding more code to the headers) is probably better.

gasche · 2024-09-20T13:17:18Z

The documentation mentions that, say, fdopen and fdclose could be annotated as allocators as well -- in general, resources can be presented as custom allocators. Maybe we would have a use for that. (But you could have generic macros and also specialized macros.)

MisterDA · 2024-09-23T14:11:40Z

I've changed the parameters to always be passed by the user. I've found two new pairs of functions to annotate.

gasche

This looks nice to me, but I would like to see more documentation before merging: this is new, and I expect it could be confusing to people reading our C code and unfamiliar with this.

The documentation should go in misc.h, where the macros are defined. It should at the very least include a URL pointing to a documentation for these weird attributes, to find out for example what the mysterious ptr_index number means. In addition, I think it would be nice if you could write a short documentation comment for each macro separately, for example something like:

/* [CAMLcalloc(dealloc, p, 1, 2)] allocates a memory block whose size
    is given by the product of its first and second argument, and must be deallocated
    by passing as the [p]-th argument of the function [dealloc(...)]. */
#define CAMLcalloc(deallocator,ptr_index,alloc_size_N,alloc_size_M,...) \
  CAMLalloc(deallocator, ptr_index)                                      \
    __attribute__ ((alloc_size(alloc_size_N,alloc_size_M)))

This helps the compiler optimize code, and static analysis by detecting potential mismatches in alloc/free pairs. - malloc > The malloc attribute indicates that the function acts like a system > memory allocation function, returning a pointer to allocated storage > disjoint from the storage for any other object accessible to the > caller. https://clang.llvm.org/docs/AttributeReference.html#malloc > Associating a function with a deallocator helps detect calls to > mismatched allocation and deallocation functions and diagnose them > under the control of options such as -Wmismatched-dealloc. It also > makes it possible to diagnose attempts to deallocate objects that > were not allocated dynamically, by -Wfree-nonheap-object. To > indicate that an allocation function both satisifies the nonaliasing > property and has a deallocator associated with it, both the plain > form of the attribute and the one with the deallocator argument must > be used. > The warnings guarded by -fanalyzer respect allocation and > deallocation pairs marked with the malloc. https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-malloc-function-attribute Note that the malloc attribute can only be applied to functions returning pointers. The OCaml value type is a typedef to an integer type, and the C compiler will refuse applying the attribute to a function returning an OCaml value. - nodiscard / warn_unused_result Prevent memory leaks by warning if the result of an allocation is ignored. https://clang.llvm.org/docs/AttributeReference.html#nodiscard-warn-unused-result - alloc_align > GCC uses this information to improve pointer alignment analysis. https://clang.llvm.org/docs/AttributeReference.html#alloc-align https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005falign-function-attribute - alloc_size > GCC uses this information to improve the results of > __builtin_object_size. https://clang.llvm.org/docs/AttributeReference.html#alloc-size https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute - returns_nonnull > lets the compiler optimize callers based on the knowledge that the > return value will never be null. > The analyzer considers the possibility that an allocation function > could fail and return null. […] If the allocator always returns > non-null, use __attribute__ ((returns_nonnull)) to suppress these > warnings. https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-returns_005fnonnull-function-attribute

MisterDA · 2024-09-27T15:15:21Z

Thanks for the review! I've added documentation. Is it a bit verbose?

gasche

The new documentation is perfect, thanks!

Octachron · 2024-10-01T14:52:46Z

As far as I can see this PR fails to compile on both too recent and too ancient version of gcc. Thus it needs more testing and I am planning to revert it soon.

gasche · 2024-10-01T15:11:09Z

Oops, sorry. Yes, please feel free to revert it. The conditionals on __has_attribute should have helped with compatibility with older versions, but obviously this does not seem to work.

This reverts commit 2f5e828, reversing changes made to 51a1781.

MisterDA · 2024-10-02T14:03:20Z

As far as I can see this PR fails to compile on both too recent and too ancient version of gcc. Thus it needs more testing and I am planning to revert it soon.

Could you point me to these failures? I've successfully build this branch with GCC 11.5 (earliest maintained release) and GCC 14.2 (latest maintained release). I'm fine with guarding the definitions with __GNUC__ >= 11.

Octachron · 2024-10-02T14:32:43Z

With ubuntu 24.04.1 and gcc 13.2.0 (so not very recent, sorry)

  CC runtime/startup_byt.b.o
In file included from /usr/include/features.h:502,
                 from /usr/include/errno.h:25,
                 from runtime/startup_byt.c:20:
In function 'read',
    inlined from 'read_section' at runtime/startup_byt.c:219:7:
/usr/include/x86_64-linux-gnu/bits/unistd.h:28:10: error: '__read_alias' specified size 18446744073709551614 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]
   28 |   return __glibc_fortify (read, __nbytes, sizeof (char),
      |          ^~~~~~~~~~~~~~~
/usr/include/x86_64-linux-gnu/bits/unistd-decl.h: In function 'read_section':
/usr/include/x86_64-linux-gnu/bits/unistd-decl.h:29:16: note: in a call to function '__read_alias' declared with attribute 'access (write_only, 2, 3)'
   29 | extern ssize_t __REDIRECT_FORTIFY (__read_alias, (int __fd, void *__buf,
      |                ^~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:1544: runtime/startup_byt.b.o] Error 1
make[1]: Leaving directory '/home/ci/builds/workspace/main/flambda/false/label/ocaml-ubuntu-latest'
make: *** [Makefile:851: world.opt] Error 2
Finished: FAILURE

For old gcc, the inria CI has seen the failure on

Red Hat Enterprise Linux release 8.9 (Ootpa)
gcc info:
| Using built-in specs.
| COLLECT_GCC=gcc
| COLLECT_LTO_WRAPPER=/usr/libexec/gcc/s390x-redhat-linux/8/lto-wrapper
| gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-20)

OCAML_4_ONLY=
CYGWIN_NT-10.0 DESKTOP-IJ4QP1H 3.3.4(0.341/5/3) 2022-01-31 19:31 i686 Cygwin
gcc info:
| Using built-in specs.
| COLLECT_AS_OPTIONS='--version'
| COLLECT_GCC=gcc
| COLLECT_LTO_WRAPPER=/usr/lib/gcc/i686-pc-cygwin/10/lto-wrapper.exe
| gcc (GCC) 10.2.0
| Copyright (C) 2020 Free Software Foundation, Inc.
| This is free software; see the source for copying conditions.  There is NO
| warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

and fails with

In file included from runtime/addrmap.c:18:
runtime/caml/memory.h:90:1: error: wrong number of arguments specified for 'malloc' attribute
   90 | CAMLextern caml_stat_block caml_stat_alloc(asize_t);
      | ^~~~~~~~~~
runtime/caml/memory.h:90:1: note: expected between 0 and 0, found 2

MisterDA · 2024-10-02T15:44:10Z

Indeed the malloc attribute was extended in the GCC 11 series:

The existing malloc attribute has been extended so that it can be used to identify allocator/deallocator API pairs. A pair of new -Wmismatched-dealloc and -Wmismatched-new-delete warnings will complain about mismatched calls, and -Wfree-nonheap-object about deallocation calls with pointers not obtained from allocation functions. Additionally, the static analyzer will use these attributes when checking for leaks, double-frees, use-after-frees, and similar issues.

and it's not possible to test with __has_attribute if its the new or the old form, but we can use the same definition of CAMLalloc for clang and for GCC pre-11.

As for the error with GCC 13, the error could be related to this bug report. GCC doesn't see that caml_seek_optional_section can only return non-negative values or -1 in case of an error, and posits that if len is negative, it might overflow the maximum object size passed to caml_stat_alloc. This can be observed with the following patch discarding negative values. Try it with docker build --ssh default ..

# syntax=docker/dockerfile:1
FROM ubuntu:24.04
ADD git@github.com:MisterDA/ocaml.git#caml_stat-annotations /tmp/ocaml
WORKDIR /tmp/ocaml
RUN apt-get -y update && apt-get install -y build-essential git
RUN ./configure --disable-ocamldoc
RUN patch -Np1 <<"EOF"
diff --git a/runtime/startup_byt.c b/runtime/startup_byt.c
index 04bb87c435..2af6089753 100644
--- a/runtime/startup_byt.c
+++ b/runtime/startup_byt.c
@@ -214,7 +214,7 @@ static char * read_section(int fd, struct exec_trailer *trail,
   char * data;

   len = caml_seek_optional_section(fd, trail, name);
-  if (len == -1) return NULL;
+  if (len <= -1) return NULL;
   data = caml_stat_alloc(len + 1);
   if (read(fd, data, len) != len)
     caml_fatal_error("error reading section %s", name);
@@ -232,7 +232,7 @@ static char_os * read_section_to_os(int fd, struct exec_trailer *trail,
   wchar_t * wdata;

   len = caml_seek_optional_section(fd, trail, name);
-  if (len == -1) return NULL;
+  if (len <= -1) return NULL;
   data = caml_stat_alloc(len + 1);
   if (read(fd, data, len) != len)
     caml_fatal_error("error reading section %s", name);
EOF
RUN make -j$(nproc)

The bug doesn't appear in GCC 14. We could either:

fix CAMLalloc for GCC pre-11 and apply the above patch; or
restrict these macros to GCC 14 or newer (not clear how that would affect clang).

Octachron · 2024-10-02T15:52:25Z

Before I forget, you should not hesitate to request an access right at Inria CI.

gasche · 2024-10-02T16:13:17Z

Maybe it's not too hard to write a small, self-contained .c program that defines and uses the macros in a way similar to the OCaml runtime itself, and then check at configure-time that this program builds correctly. This may let us distinguish between versions-that-work and versions-that-don't-work with less conditionals on specific version numbers?

MisterDA · 2024-10-08T13:21:19Z

Well, the second problem seems to be a limitation of the compiler's static analyzer. It's not possible to account for all past (and future!) compiler bugs… but I can build this branch with GCC 14 and any version of clang, and I propose to keep this restriction with the hope that if future GCC warns, it'll be a useful warning.

MisterDA force-pushed the caml_stat-annotations branch from f82de5d to de5bc27 Compare September 23, 2024 13:49

MisterDA changed the title ~~Annotate caml_stat_* functions with compiler attributes~~ Annotate alloc/free open/close functions with compiler attributes Sep 23, 2024

MisterDA force-pushed the caml_stat-annotations branch from de5bc27 to 029f83e Compare September 23, 2024 14:13

gasche approved these changes Sep 25, 2024

View reviewed changes

MisterDA added 2 commits September 27, 2024 17:12

Document new annotation macros

b516b1a

MisterDA force-pushed the caml_stat-annotations branch from 029f83e to b516b1a Compare September 27, 2024 15:14

gasche approved these changes Sep 27, 2024

View reviewed changes

gasche added the merge-me label Sep 27, 2024

Merge branch 'trunk' into caml_stat-annotations

bf93c1b

gasche merged commit 2f5e828 into ocaml:trunk Oct 1, 2024

MisterDA deleted the caml_stat-annotations branch October 1, 2024 13:42

Octachron added a commit that referenced this pull request Oct 1, 2024

Revert "Merge pull request #13457 from MisterDA/caml_stat-annotations"

bce3bb7

This reverts commit 2f5e828, reversing changes made to 51a1781.

MisterDA restored the caml_stat-annotations branch October 2, 2024 13:13

MisterDA mentioned this pull request Oct 9, 2024

Annotate alloc/free open/close functions with compiler attributes #13537

Merged

Conversation

MisterDA commented Sep 19, 2024

Uh oh!

gasche commented Sep 19, 2024

Uh oh!

MisterDA commented Sep 20, 2024

Uh oh!

gasche commented Sep 20, 2024

Uh oh!

MisterDA commented Sep 23, 2024

Uh oh!

gasche left a comment

Choose a reason for hiding this comment

Uh oh!

MisterDA commented Sep 27, 2024

Uh oh!

gasche left a comment

Choose a reason for hiding this comment

Uh oh!

Octachron commented Oct 1, 2024

Uh oh!

gasche commented Oct 1, 2024

Uh oh!

MisterDA commented Oct 2, 2024

Uh oh!

Octachron commented Oct 2, 2024

Uh oh!

MisterDA commented Oct 2, 2024

Uh oh!

Octachron commented Oct 2, 2024

Uh oh!

gasche commented Oct 2, 2024

Uh oh!

MisterDA commented Oct 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants