ludocode / mpack

MPack - A C encoder/decoder for the MessagePack serialization format / msgpack.org[C]
MIT License
533 stars 82 forks source link

Error with mpack_writer_destroy when no data has been written #58

Closed volks73 closed 5 years ago

volks73 commented 6 years ago

I have been working with the mpack library, and I have come across an error. It appears to be related to using the mpack_writer_destroy function when no other mpack_write_* functions have been used. In other words, an error occurs destroying a writer with no data. I understand this is an edge case because why would one create and then ultimately destroy a writer without using it? However, it feels like mpack should exit cleanly/safely as opposed to causing an abort under these conditions.

I have created a simplified example that reproduces the problem on my machine. I was not sure how to convert it to a test to include with the mpack test suite. Instead, I have included the example as an attachment, along with a screen capture of the error dialog that appears (Error-Dialog.png). Below is the source code for the main function that reproduces the problem, which is located in issue.c of the attached archive:

#include "mpack.h"

int 
main(int argc, char* argv[])
{
    char* data = NULL;
    size_t size = 0;
    mpack_writer_t* writer = malloc(sizeof(mpack_writer_t));
    if (!writer) {
        fprintf(stderr, "A memory-related error occurred!\n");
        return 1;
    }
    mpack_writer_init_growable(writer, &data, &size);
    if (mpack_writer_error(writer) != mpack_ok) {
        fprintf(stderr, "An initialization-related error occurred!\n");
        return 1;
    }
    //mpack_write_int(writer, 56); <-- Uncomment this line, i.e. write some data, to avoid the issue
    if (mpack_writer_destroy(writer) != mpack_ok) {
        fprintf(stderr, "A destruction-related error occurred!\n");
        return 1;
    }
    fprintf(stdout, "Data:");
    for (size_t i = 0; i < size; i++) {
        fprintf(stdout, " %X", data[i]);
    }
    fprintf(stdout, "\n");
    free(data);
    free(writer);
    return 0;
}

The error does not appear to occur on macOS High Sierra. It only occurs on Windows 10, where I am using the MSVC compiler included with Microsoft C++ Build Tools 2017. I have not tried it on Linux. I have created a CMakeLists.txt file to help with building the example and included it in the attached archive file. I am using mpack v0.8.2 with the default configuration (mpack-config.h) in the example (also included in the attached archive). The following instructions can be used to build the example demonstrating the issue:

  1. Download the attached archive.
  2. Extract the archive.
  3. Start the x64 Native Build Tools command prompt that should have been included with Microsoft C++ Build Tools 2017.
  4. Navigate to the extracted archive folder.
  5. Run the following commands to build and run the example:
mkdir build
cd build
cmake ..
cmake --build .
Debug\run_issue.exe

I believe I have narrowed down the problem to the mpack_growable_writer_teardown function and its use of the mpack_realloc function, but I am not sure on how to resolve the issue, or if this is even an error. Maybe it is just a configuration I need to change? It is very possible I have missed something.

issue.zip

error-dialog

ludocode commented 6 years ago

Thanks for the excellent bug report.

This bug has been fixed on develop back in April in 083e28dd09a5dbf534d5c2eac0a7d220235dbac0. Unfortunately it's been far too long since I've done a release, so it's still unfixed in the latest release version. I'm still working on a new release, but I've been delaying it since I keep trying to add more features (most recently timestamp support, which you can see in the timestamp branch here.)

In the meantime you could upgrade to the develop branch, or cherry-pick that commit over to the v0.8.1 tag. The tools/amalgamate.sh script can be used to generate new mpack.h and mpack.c files.

volks73 commented 6 years ago

Great! I am glad it was already resolved, and I am sorry for the redundant issue.

In the meantime you could upgrade to the develop branch, or cherry-pick that commit over to the v0.8.1 tag. The tools/amalgamate.sh script can be used to generate new mpack.h and mpack.c files.

Thanks for the temporary workaround. I look forward to the next revision/release.

ludocode commented 5 years ago

Oops, forgot to close this back when I released MPack 1.0. This is fixed in the latest release.