Fix a few issues with the C generator (part 8)

[eafer]

This is the final patch from the original pull request at #14379. I left it for last because I think it might be controversial.

The problem is as follows. The C generator produces apparently matching *_create() and *_free() functions, but they can't actually be used together. *_create() only allocates memory for the object itself, not for its internal fields (it's a "shallow copy", so to speak), but *_free() will free everything. The caller may have allocated some of those fields statically, or on the stack, or they may still need them. Even if they used malloc(), there is no reason to believe that it's the exact same malloc() implementation from the library. All very confusing for the users, and potentially exploitable.

To deal with this as simply as possible (and without affecting other users of the generator if possible), this pull request includes the following changes:

1. Mark *_create() as deprecated to discourage any callers. This function is useless: it just allocates the memory for the object and sets the fields, which the callers can do themselves without any extra work. The caller may even choose to allocate the whole object on the stack in many cases.
2. Add a new _library_owned field to objects, that only gets set by *_create(). Use this to tell apart objects allocated by the library from those allocated by the caller; refuse to free the latter and print a warning. This isn't perfect because, depending on how the allocation took place, this field may have some stale data; but it's better than before and the user will see the warning eventually.
3. Define an internal version of *_create() for use by the library itself. The point is to prevent the deprecation warnings from showing up during the library build.

I appreciate that this is a huge mess, but we believe it's necessary for security and to make life easier for our users. Any ideas on simpler ways to deal with the problem are of course very welcome.

@wing328 @ityuhui @zhemant @michelealbano

[imaami]

Can I jump in to discuss library API design choices? I would suggest a specific way to implement both initialization and allocation + their inverse. I'll try to summarize it briefly.

For every object type:

• There should be an init/deinit function pair. These accept a pointer to an allocated object. They don't handle allocating or freeing the object itself. Their job is to init/deinit the internal data, including allocating and freeing of nested data pointers, if any. These are meant for on-stack object management and for users who choose their own allocation method.
• There should be a create/destroy function pair. These are wrappers around the init/deinit functions which also manage allocation and deallocation.

object.h

#ifndef PROJECTNAME_OBJECT_H_
#define PROJECTNAME_OBJECT_H_

#include <stdbool.h>

// Object type definition.
struct object {
    int a;
    bool b;
};

// Initialize an allocated object. Returns an `errno` code on failure.
extern int
object_init (struct object *obj,
             int            some_param,
             bool           another_param);

// Deallocate any possible internal allocations and wipe all data.
extern void
object_fini (struct object *obj);

// Allocate a new object, initialize it by calling `object_init()`, and return a
// pointer to the new object. If something fails everything is rolled back
// (internal allocations are freed), an error code is saved to `*err` if `err`
// is not `nullptr`, and `nullptr` is returned.
//
// Note: don't read `err` if the return value is not `nullptr`. An uninitialized
// `err` passed to `object_create()` remains uninitialized if `object_create()`
// succeeds.
extern struct object *
object_create (int   some_param,
               bool  another_param,
               int  *err);

// Unilitialize and free the object pointed to by `*objp` and clear the user's
// object pointer. (Double frees are a common category of bugs in C code,
// and the library should ease that burden if possible.)
//
// Note: passing `nullptr` or a pointer to `nullptr` is a NOP.
extern void
object_destroy (struct object **objp);

#endif // PROJECTNAME_OBJECT_H_

object.c

#include <errno.h>
#include <stdlib.h>
#include <string.h>

#include "object.h"

// C23 compat hack
#undef HAVE_C23_NULLPTR
#if __STDC_VERSION__ >= 202000L
# if defined __clang_major__
#  if __clang_major__ >= 16
#   define HAVE_C23_NULLPTR 1
#  endif
# elif defined __GNUC__
#  if (__GNUC__ > 13) || (__GNUC__ == 13 && __GNUC_MINOR__ >= 1)
#   define HAVE_C23_NULLPTR 1
#  endif
# endif
#endif
#ifndef HAVE_C23_NULLPTR
# define nullptr NULL
#endif

int
object_init (struct object *obj,
             int            some_param,
             bool           another_param)
{
    if (!obj)
        return EFAULT;

    // memset() zeroes both data and internal padding bytes
    memset(obj, 0, sizeof *obj);

    obj->a = some_param;
    obj->b = another_param;

    return 0;
}

void
object_fini (struct object *obj)
{
    if (obj) {
        // no allocations to free this time
        memset(obj, 0, sizeof *obj);
    }
}

struct object *
object_create (int   some_param,
               bool  another_param,
               int  *err)
{
    struct object *obj = malloc(sizeof *obj);
    int e = errno;
    if (!obj)
        // allocation failure
        goto fail_alloc;

    e = object_init(obj, some_param, another_param);
    if (!e)
        // no errror
        return obj;

    // init failed
    free(obj);

fail_alloc:
    if (err)
        *err = e;

    return nullptr;
}

void
object_destroy (struct object **objp)
{
    if (objp) {
        struct object *obj = *objp;
        *objp = nullptr;
        object_fini(obj);
        free(obj);
    }
}

[imaami]

To clarify: that example design isn't necessarily meant as a change suggestion specifically for this exact PR. Implementing it across the board for the C generator would be a larger undertaking (although not a huge effort).

[eafer]

I don't disagree, but I'm trying to keep the api changes as small as possible to avoid annoying other users of the generator. Also, we sometimes allocate simple objects on the stack like this: https://github.com/ARM-software/avh-api/blob/c7a6b1f8a71c235a684766eac44adb3a9b9d80a1/c/example/example.c#L427, which is convenient. My fear is that having too many different ways to allocate/init the objects could confuse the users.

[imaami]

It does seem like there's asymmetry between the current create and free interfaces. But what's the long-term plan for memory management? Leaving everything up to the library users is probably not going to help with security. Or maybe I misunderstood what you meant?