make Tox_Options self contained

[sudden6]

tox.h https://github.com/TokTok/c-toxcore/blob/master/toxcore/tox.h#L557 says:

The data pointed at by this member is owned by the user, so must outlive the options object.

For savedata_data and proxy_host. I think we should change this so that the setter function copies the data and the destructor frees the memory. Optionally we can could also take ownership of the pointer passed and only free it in the destructor, but I don't think this is a good design practice.

This doesn't break the API, so it should be safe to implement.

[iphydf]

Agreed. We should allocate and make a copy.

[nurupo]

I'm not sure if this is a good idea. Tox_Options is supposed to be short-lived, you just set things on it, create a Tox instance with it and free it right away. At the very least I suggest postponing this until we change Tox_Options to an opaque struct pointer in the API, as we have faced some issues in #909 trying to add copying of the members. Specifically, if using the setter function -- it will malloc some of the members, which requires tox_options_free() to be called for toxcore to free them, meaning that the user must also have allocated the Tox_Options struct with tox_options_new() before using the setter since tox_options_free() will try to free Tox_Options itself too, but user might have allocated the Tox_Options struct on stack instead and used the setter, which would malloc a struct member and copy into it, but then the user wouldn't call tox_options_free() to free the memory malloced by toxcore, which would result in memory leak. Oh, and even if the user did call tox_options_free() in this case, toxcore will also try to free Tox_Options which was allocated by the user on the stack, which is very bad. Another case if that the user might have allocated the struct with tox_options_new(), but set the struct member directly, without using the setter setter that does malloc+memcpy, and then called tox_options_free(), which will assume user did use the setter and some struct members were malloced and will try to free whatever user has set, which might have been a stack-allocated by the user, which is a very bad thing to free. All these problems go away if the struct becomes opaque.

[nurupo]

Here is some code for the 3 examples from my previous comment. I have also come up with an additional 4th example.

Tox* make_tox(void)
{
    Tox_Options opt;
    tox_options_default(&opt);
    File* f = file_open("~./config/tox/tox.savedata");
    size_t length = file_length(f);
    uint8_t savedata[length];
    file_readall(f, &savedata);
    tox_options_set_savedata_data(&opt, data, length); // <-- mallocs Tox_Options.savedata internally
    Tox* tox = tox_new(&opt, NULL);
    file_close(f);
    return tox;
} // <-- memory leak, Tox_Options.savedata never freed

Alright, let's try to avoid the memory leak by freeing the memory with tox_options_free() call:

Tox* make_tox(void)
{
    Tox_Options opt;
    tox_options_default(&opt);
    File* f = file_open("~./config/tox/tox.savedata");
    size_t length = file_length(f);
    uint8_t savedata[length];
    file_readall(f, &savedata);
    tox_options_set_savedata_data(&opt, data, length); // <-- mallocs Tox_Options.savedata internally
    Tox* tox = tox_new(&opt, NULL);
    tox_options_free(&opt); // <-- frees Tox_Options.savedata so no memory leak,
                            //     but also frees stack-allocated Tox_Options!
    file_close(f);
    return tox;
}

Alright, allocating Tox_Options on stack and using the setter always leads to bugs, so how about not doing so. I'm also tired of using tox_options_set_savedata_data(), let's change things up and directly assign to savedata_data struct member:

Tox* make_tox(void)
{
    Tox_Options* opt = tox_options_new(NULL);
    File* f = file_open("~./config/tox/tox.savedata");
    size_t length = file_length(f);
    uint8_t savedata[length];
    file_readall(f, &savedata);
    opt->savedata_data = savedata;
    opt->savedata_length = length;
    Tox* tox = tox_new(opt, NULL);
    tox_options_free(opt); // <-- frees stack-allocated Tox_Options.savedata!
    file_close(f);
    return tox;
}

Wow, still buggy. Alright, I'm not sure if allocating the file on stack is smart, it might blow up the stack when the file grows big, so let's allocate it on heap instead:

Tox* make_tox(void)
{
    Tox_Options* opt = tox_options_new(NULL);
    File* f = file_open("~./config/tox/tox.savedata");
    uint8_t* savedata = file_readall_malloc(f);
    size_t length = file_length(f);
    opt->savedata_data = savedata;
    opt->savedata_length = length;
    Tox* tox = tox_new(opt, NULL);
    tox_options_free(opt); // <-- frees opt->savedata_data, everything is okay-ish
    free(savedata); // <-- double free of savedata!
    file_close(f);
    return tox;
}

Welp, no dice either.

[sudden6]

How I imagine things will work:

Tox* make_tox(void)
{
    Tox_Options* opt = tox_options_new();
    File* f = file_open("~./config/tox/tox.savedata");
    uint8_t* savedata = file_readall_malloc(f);
    size_t length = file_length(f);
    tox_options_set_savedata_data(&opt, data, length); // <-- mallocs Tox_Options.savedata internally
    file_close(f);
    free(savedata); // free savedata_data
    Tox* tox = tox_new(opt, NULL);
    tox_options_free(opt); // <-- frees opt->savedata_data, everything is okay
    return tox;
}

This works best if Tox_Options is opaque.

[nurupo]

That's what I'm saying -- we first need to make Tox_Options opaque before introducing the copying, or introduce those two at the same time. Without opaque Tox_Options the API doesn't enforce the correct use, there are many seemingly correct ways to use the API which result in bugs like double free or memory leaks, just as my examples have shown.

[sudden6]

Ok, my original idea was to introduce this without breaking the API, but now it seems this is not possible and I'll wait for the next API update.