amazon-ion / ion-c

A C implementation of Amazon Ion.
https://amazon-ion.github.io/ion-docs/
Apache License 2.0
166 stars 43 forks source link

User buffer backed writer exceeds buffer length when writing. #319

Open nirosys opened 1 year ago

nirosys commented 1 year ago

When creating an ION_WRITER backed by a user supplied buffer, it appears the stream limit continues to increase beyond the stream size (as if it were a page-backed stream).

Address 0x7ff7b4a06aaa is located in stack of thread T0 at offset 42 in frame
    #0 0x10b8c52bf in IonTextDownconvert_ErrorIfBufferSmall_Test::TestBody() test_ion_text.cpp:1423

  This frame has 15 object(s):
    [32, 42) 'json_text' (line 1426) <== Memory access at offset 42 overflows this variable
    [64, 68) 'err' (line 1428)
    [80, 88) 'reader' (line 1429)
    [112, 120) 'writer' (line 1430)
    [144, 256) 'woptions' (line 1431)
    [288, 360) 'roptions' (line 1432)

Using the unit test:

TEST(IonTextDownconvert, ErrorIfBufferSmall) {
    const char *ion_text = "{data: null.string}";
    // JSON should be '{"data": null}'
    char json_text[10]; // would need 14 bytes to write the JSON.

    iERR err = IERR_OK;
    hREADER reader = NULL;
    hWRITER writer = NULL;
    ION_WRITER_OPTIONS woptions = {0};
    ION_READER_OPTIONS roptions = {0};

    // woptions.json_downconvert = TRUE;
    memset((void*)json_text, 0, sizeof(json_text));

    IONCHECK(ion_reader_open_buffer(&reader, (BYTE*)ion_text, strlen(ion_text), &roptions));
    IONCHECK(ion_writer_open_buffer(&writer, (BYTE*)json_text, sizeof(json_text) - 1, &woptions));

    ASSERT_TRUE(_ion_stream_is_fully_buffered(writer->output));
    // This call should return an IERR_EOF error, indicating that the writer has reached its end.
    err = ion_writer_write_all_values(writer, reader);

    ASSERT_EQ(err, IERR_EOF);
    ion_writer_close(writer);
    ion_reader_close(reader);
    return;
fail:
    if (writer != NULL)
        ion_writer_close(writer);
    if (reader != NULL)
        ion_reader_close(reader);
    ASSERT(false);
}

I started digging into this and it looks like there is a conflict between how paged streams are treated, vs user buffer streams. The limit appears to be extending as bytes are writing to the stream and the size of the buffer is not considered at some point. I need to keep digging.