Question about Implementing drop on an FFI library and preventing undefined behavior

Question

I am trying to build a rust wrapper around a c library. For the structs in this library there are generally two functions for memory management, one for allocation and another for deallocation.

Now I plan to use the allocation code in the constructor of the wrapper type and the destructor in the drop trait for these types. However there's a problem, on some functions in the c library where we actually consume these structs, I see that these deallocation functions are being called within these functions. I fear that if I were to call one of these functions then allow the wrapper struct to go out of scope the deallocation code would run twice which is undefined behavior.

Is there a way to ensure drop isn't called for a block of code? Or somehow prevent this code from being called twice when these functions are called?

Here's the struct I am referring to picture.h

#ifndef __VMAF_PICTURE_H__
#define __VMAF_PICTURE_H__

#include <stddef.h>

#ifdef __cplusplus
extern "C" {
#endif

enum VmafPixelFormat {
    VMAF_PIX_FMT_UNKNOWN,
    VMAF_PIX_FMT_YUV420P,
    VMAF_PIX_FMT_YUV422P,
    VMAF_PIX_FMT_YUV444P,
    VMAF_PIX_FMT_YUV400P,
};

typedef struct VmafRef VmafRef;

typedef struct {
    enum VmafPixelFormat pix_fmt;
    unsigned bpc;
    unsigned w[3], h[3];
    ptrdiff_t stride[3];
    void *data[3];
    VmafRef *ref;
} VmafPicture;

int vmaf_picture_alloc(VmafPicture *pic, enum VmafPixelFormat pix_fmt,
                       unsigned bpc, unsigned w, unsigned h); // This function will allocate memory for a pointer to VmafPicture

int vmaf_picture_unref(VmafPicture *pic); // This will free memory of this pointer

#ifdef __cplusplus
}
#endif

#endif /* __VMAF_PICTURE_H__ */

And the function in question:

int vmaf_read_pictures(VmafContext *vmaf, VmafPicture *ref, VmafPicture *dist,
                       unsigned index)
{
    if (!vmaf) return -EINVAL;
    if (vmaf->flushed) return -EINVAL;
    if (!ref != !dist) return -EINVAL;
    if (!ref && !dist) return flush_context(vmaf);

    int err = 0;

    vmaf->pic_cnt++;
    err = validate_pic_params(vmaf, ref, dist);
    if (err) return err;

    if (vmaf->thread_pool)
        return threaded_read_pictures(vmaf, ref, dist, index);

    for (unsigned i = 0; i < vmaf->registered_feature_extractors.cnt; i++) {
        VmafFeatureExtractorContext *fex_ctx =
            vmaf->registered_feature_extractors.fex_ctx[i];

        if ((vmaf->cfg.n_subsample > 1) && (index % vmaf->cfg.n_subsample) &&
            !(fex_ctx->fex->flags & VMAF_FEATURE_EXTRACTOR_TEMPORAL))
        {
            continue;
        }

        err = vmaf_feature_extractor_context_extract(fex_ctx, ref, NULL, dist,
                                                     NULL, index,
                                                     vmaf->feature_collector);
        if (err) return err;
    }

// Pictures are deallocated here!
    err = vmaf_picture_unref(ref);
    if (err) return err;
    err = vmaf_picture_unref(dist);
    if (err) return err;

    return 0;
}

And here's the code for vmaf_picture_unref. To tell the truth I have no idea what's happening here as I am unfamiliar with c

int vmaf_picture_unref(VmafPicture *pic) {
    if (!pic) return -EINVAL;
    if (!pic->ref) return -EINVAL;

    vmaf_ref_fetch_decrement(pic->ref);
    if (vmaf_ref_load(pic->ref) == 0) {
        aligned_free(pic->data[0]);
        vmaf_ref_close(pic->ref);
    }
    memset(pic, 0, sizeof(*pic));
    return 0;
}

Do you have some code for context? A lot of C interfaces use pointers, which are handled via the `unsafe` mechanics where you do the memory management yourself. — tadman, Nov 26 '22 at 02:07
Sure can, I'll share some of the c code that I'm referring to — Brandon Piña, Nov 26 '22 at 02:08
It's the Rust code where you seem to be having the problems, as C does not "drop" anything on you automatically. Things just fall out of scope. — tadman, Nov 26 '22 at 02:12
Exactly what i'm getting at. I can't control that the memory is being freed inside vmaf_read_frames, so if I implement the drop trait for the wrapper type that calls the function that frees this memory, that would essentially mean that the function is called twice if I were to ever use this function which is undefined behavior — Brandon Piña, Nov 26 '22 at 02:16
Your job as a wrapper author is to decide which functions to expose, and which ones to incorporate as an implicit part of the implementation contract. If you have a wrapper type, I'd expect it takes care of releasing memory on `drop`. I would *not* expect to have a direct call to the low-level release function since calling it would only ever break things. — tadman, Nov 26 '22 at 02:17
So in my implementation of drop, I wouldn't call vmaf_picture_unref? — Brandon Piña, Nov 26 '22 at 02:20
Your implementation of the wrapper *would* call it, but the wrapper itself would not expose it as a callable function to the library's user. — tadman, Nov 26 '22 at 02:21
That's what I was asking. If multiple calls to vmaf_pic_unref would result in undefined behavior and the drop trait would call my implementation which would call vmaf_pic_unref implicitly whenever it goes out of scope, what happens if I were to call vmaf_read_picture then allow my vmaf picture to go out of scope? — Brandon Piña, Nov 26 '22 at 02:23
You're going to need to understand your responsibility for memory allocation very clearly before closing this wrapper out. If a function like that releases memory on the arguments it's called with, then that translates to a Rust function that *consumes* those arguments, as in it does not take references, but values. As a consequence, those values get dropped, so you may need to ensure that you have some kind of internal-use function that nulls out the allocation beforehand, so drop doesn't do any damage. For example, your wrapper object has `Option`. — tadman, Nov 26 '22 at 02:25
In your case, perhaps `vmaf_read_pictures(vmaf: &VmafContext, ref: VmafPicture, dist: VmafPicture, index: usize)` — tadman, Nov 26 '22 at 02:25
Does this mean that drop is only called when a *reference* to a value goes out of scope rather than the value itself? — Brandon Piña, Nov 26 '22 at 02:28
Drop is only called when a value goes out of scope. References imply ownership is held elsewhere. — tadman, Nov 26 '22 at 02:29
Which brings us back to the original question, how do I reconcile releasing this memory in the drop trait which is called implicitly when a value goes out of scope with this function in this external library doing so internally on certain function calls which I can not control — Brandon Piña, Nov 26 '22 at 02:34
How are you wrapping this allocation, that's the real question. If you use an `Option` then you can have your own function to "mark as released" that just calls `take()` on that to effectively delete it. Within your `Drop` implementation you check if there's a pointer to delete, and if there's none, just move on. I'd suggest something like `VmafPicture` having a `fn deallocated()` which does the dirty work, explicitly not set to `pub`. — tadman, Nov 26 '22 at 02:36
You could even have this as a private trait so as not to clutter up your documentation, etc. if this is a pervasive pattern within this library, or this might manifest as your own pointer wrapper `enum` that makes this more self-contained. — tadman, Nov 26 '22 at 02:37
From the `vmaf_picture_unref` code, it is clear that your pictures are reference-counted, so there is probably also a `vmaf_picture_ref` function which you haven't show us. Another way to solve the problem is to call `vmaf_picture_ref` before calling `vmaf_read_pictures` to keep it from freeing the pictures (although I'd argue that there's a bug in `vmaf_read_pictures` because it only unrefs the pictures when successful and not on error). — Jmb, Nov 26 '22 at 09:47

Question about Implementing drop on an FFI library and preventing undefined behavior

0 Answers0