[c] - cabi_realloc looks like its leaking memory:

[GordonSmith]

I am looking at: https://github.com/bytecodealliance/wit-bindgen/blob/main/crates/c/src/lib.rs#L917-L923

                __attribute__((__weak__, __export_name__("cabi_realloc")))
                void *cabi_realloc(void *ptr, size_t old_size, size_t align, size_t new_size) {
                    (void) old_size;
                    if (new_size == 0) return (void*) align;
                    void *ret = realloc(ptr, new_size);
                    if (!ret) abort();
                    return ret;
                }

When new_size == 0 I would expect it to either free the memory, or to realloc to 0 size (same effect?). Why is it returning a pointer to the first or second byte in the linear memory?

Also shouldn't the realloc size be multiplied by the align size?

This is what I would have expected:

                __attribute__((__weak__, __export_name__("cabi_realloc")))
                void *cabi_realloc(void *ptr, size_t old_size, size_t align, size_t new_size) {
                    void *ret = realloc(ptr, align * new_size);
                    if (!ret) abort();
                    return ret;
                }

[alexcrichton]

To answer your questions:

When new_size == 0 I would expect it to either free the memory, or to realloc to 0 size (same effect?).

The canonical ABI does not use cabi_realloc to free anything, so this never happens, so it doesn't need to be checked.

Why is it returning a pointer to the first or second byte in the linear memory?

Because zero-sized allocations are not allowed to return a null pointer, and the pointer must always be aligned.

Also shouldn't the realloc size be multiplied by the align size?

No, it's assumed that wasi-libc's realloc has a minimum alignment which is greater than align.

Your replacement doesn't correctly handle zero-sized allocations and it allocates too much since new_size does not need to factor in align.

[GordonSmith]

In the canonical ABI we have: https://github.com/WebAssembly/component-model/blob/main/design/mvp/CanonicalABI.md?plain=1#L268

class CanonicalOptions:
  memory: bytearray
  string_encoding: str
  realloc: Callable[[int,int,int,int],int]  <-----------------------------
  post_return: Callable[[],None]

I had been resolving the cabi_realloc and using it in the CanonicalOptions - is that incorrect?

[alexcrichton]

Sorry I'm not sure what you mean? Yes realloc is there, but can you clarify how you've been using it? It's true that canonical options point to a realloc (optionally, it doesn't have to be specified in all cases), but the canonical ABI only has a fixed set of use cases for the function and freeing allocations is not part of those use cases.

[GordonSmith]

FWIW I had been tweaking the encode / decode calls to avoid having to memcpy the encoded value in the host and then immediately memcpy it again into the guest memory and I guess I had been "misusing" that call to alloc / free while testing a few approaches. Now that I know the rules all should be good - another example of a bad assumption... Thanks for clarifying!

[alexcrichton]

Ah ok, I should clarify that my comments above reflect the current state of the spec, but if you're looking to change the spec itself then the current definition of cabi_realloc could become invalid, yes. I think that's ok but it's something we'd want to take into account when changing the spec.