dvidelabs / flatcc

FlatBuffers Compiler and Library in C for C
Apache License 2.0
646 stars 182 forks source link

vtable offset out of range or unaligned #219

Closed hinxx closed 11 months ago

hinxx commented 2 years ago

I'm trying to 'port' C++ based flatbuffer generator to C and I'm getting this error when verifying the buffer:

LogData buffer is invalid: vtable offset out of range or unaligned

C++ and C buffer hexdumps are not of the same size; there are two bytes of difference. When C generated buffer is fed to the python reader if fails to recognize the buffer, while C++ generated buffer is correctly parsed. I'm not the author of the C++/python implementations and I new to flatbuffers so excuse my superficial understanding of the subject ; I might be doing something wrong in C code. C++/python implementations are in productions use and can't be changed so I need to figure out how to make C one work with like those two.

C generated buffer size is 110 bytes:

0000:   0C 00 00 00 66 31 34 32   00 00 00 00 AE FF FF FF    ....f142........
0010:   4E 61 BC 00 00 00 00 00   20 00 00 00 3C 00 00 00    Na...... ...<...
0020:   03 00 01 00 0A 00 00 00   00 00 00 00 D4 FF FF FF    ................
0030:   00 00 00 00 00 00 F0 3F   1A 00 00 00 4C 41 42 3A    .......?....LAB:
0040:   64 65 74 31 30 3A 41 72   72 61 79 43 6F 75 6E 74    det10:ArrayCount
0050:   65 72 5F 52 42 56 00 00   06 00 0C 00 04 00 10 00    er_RBV..........
0060:   19 00 0C 00 18 00 10 00   04 00 14 00 16 00          ..............  

C++ generated buffer size is 112 bytes:

0000:   1C 00 00 00 66 31 34 32   00 00 00 00 10 00 20 00    ....f142...... .
0010:   0C 00 07 00 10 00 14 00   08 00 0A 00 10 00 00 00    ................
0020:   00 00 00 0A 03 00 01 00   14 00 00 00 38 00 00 00    ............8...
0030:   4E 61 BC 00 00 00 00 00   00 00 00 00 1A 00 00 00    Na..............
0040:   4C 41 42 3A 64 65 74 31   30 3A 41 72 72 61 79 43    LAB:det10:ArrayC
0050:   6F 75 6E 74 65 72 5F 52   42 56 00 00 00 00 06 00    ounter_RBV......
0060:   0C 00 04 00 06 00 00 00   00 00 00 00 00 00 F0 3F    ...............?

My schema has a union of tables in a root table.

table Double { value:  double; }

union Value {
 Double,
}

enum AlarmStatus: ushort {
NO_CHANGE,
HIHI,
...
}

enum AlarmSeverity: ushort {
NO_CHANGE,
MAJOR
...
}

table LogData {
 source_name: string;
 value: Value;
 timestamp: ulong;
 status: AlarmStatus = NO_CHANGE;
 severity: AlarmSeverity = NO_CHANGE;
}

root_type LogData;

I've omitted other data types for brevity.

C code that generates the buffer:

    flatbuffers_string_ref_t pv = flatbuffers_string_create_str(B, vame);
    Double_ref_t val = LogData_value_Double_create(B, 1.0);
    Value_union_ref_t un = Value_as_Double(val);
    logData = LogData_create_as_root(B, pv, un, 12345678, AlarmStatus_HIHI, AlarmSeverity_MAJOR);

I also noticed that if I use different (smaller) string lengths, the buffer size reduces in 8 byte steps in both cases, but the C buffer is not aligned to neither 4 nor 8 (ie. 110, 102, ..), while C++ buffer is 8 byte aligned (112, 104,..). Similar behavior is observed when length of the string is increased.

I've been reading #210 and #213 as it feels like my issues are related, but I can not see what the solution is.

mikkelfj commented 2 years ago

Yeah, it sounds like they are related to the mentioned issue, that was also my first thought. Are you verying with C++ and generating with C? If, so you can try pad the buffer with extra zeros up to an aligned boundary, e.g. 8 bytes, and try again.

I have not looked closely at your buffer yet, but that would be the first step to test.

As to Python, I don't think it cares about buffer size alignment, but I am not sure. In the past Python has failed because it did not understand valid buffers with file identifiers, but I should think that has been fixed. Anyway, you can generete a C buffer without identifier either by removing it from the schema or by using the _with_identifier variant call when creating the buffer, and use NULL as the identifier.

mikkelfj commented 2 years ago

You can also try to run the C verifier, just to rule out any blatant mistakes in building the buffer.

Your code looks fine from a cursory look. Note that the approach you use cannot be used for nested buffers because buffer data like the string and union must then be within the buffer start call which is hidden insided the create_as_root call.

Looking at the buffer hex, I doublt they contain a file identifier, or if they do, C++ also has it, and then the python problem shoud be the same there.

hinxx commented 2 years ago

Are you verying with C++ and generating with C? If, so you can try pad the buffer with extra zeros up to an aligned boundary, e.g. 8 bytes, and try again.

No. I'm calling flatcc verify_as_root_with_identifier(). I did not get that far to verify with C++ as flatcc itself reports a problem. I'll try to verify the C generated buffer using C++ verifier, too.

Here are the buffers, first one is un-padded and second one is manually padded with two 0 bytes. Same error by the the flatcc verifier is reported:

fb size 110
0000:   0C 00 00 00 66 31 34 32   00 00 00 00 AE FF FF FF    ....f142........
0010:   4E 61 BC 00 00 00 00 00   20 00 00 00 3C 00 00 00    Na...... ...<...
0020:   03 00 01 00 0A 00 00 00   00 00 00 00 D4 FF FF FF    ................
0030:   00 00 00 00 00 00 F0 3F   1A 00 00 00 4C 41 42 3A    .......?....LAB:
0040:   64 65 74 31 30 3A 41 72   72 61 79 43 6F 75 6E 74    det10:ArrayCount
0050:   65 72 5F 52 42 56 00 00   06 00 0C 00 04 00 10 00    er_RBV..........
0060:   19 00 0C 00 18 00 10 00   04 00 14 00 16 00          ..............  
LogData buffer is invalid: vtable offset out of range or unaligned
fb size 112
0000:   0C 00 00 00 66 31 34 32   00 00 00 00 AE FF FF FF    ....f142........
0010:   4E 61 BC 00 00 00 00 00   20 00 00 00 3C 00 00 00    Na...... ...<...
0020:   03 00 01 00 0A 00 00 00   00 00 00 00 D4 FF FF FF    ................
0030:   00 00 00 00 00 00 F0 3F   1A 00 00 00 4C 41 42 3A    .......?....LAB:
0040:   64 65 74 31 30 3A 41 72   72 61 79 43 6F 75 6E 74    det10:ArrayCount
0050:   65 72 5F 52 42 56 00 00   06 00 0C 00 04 00 10 00    er_RBV..........
0060:   19 00 0C 00 18 00 10 00   04 00 14 00 16 00 00 00    ................
LogData buffer is invalid: vtable offset out of range or unaligned

If was playing around with flatbuffers_buffer_start_aligned() like so:

    flatbuffers_buffer_start_aligned(B, LogData_file_identifier, 4);
    flatbuffers_string_ref_t pv = flatbuffers_string_create_str(B, vame);
    Double_ref_t val = LogData_value_Double_create(B, 1.0);
    Value_union_ref_t un = Value_as_Double(val);
    logData = LogData_create(B, pv, un, 12345678, AlarmStatus_HIHI, AlarmSeverity_MAJOR);
    flatbuffers_buffer_end(B, logData);

This results in the 112 long buffer, same as the second one above. The buffer also does not pass the verification.

You can also try to run the C verifier, just to rule out any blatant mistakes in building the buffer.

That was done in initial report, yes. It is the C verifier that says buffer is invalid.

Your code looks fine from a cursory look. Note that the approach you use cannot be used for nested buffers because buffer data like the string and union must then be within the buffer start call which is hidden insided the create_as_root call.

I'm not sure if your are saying that I'm fine in using the create_as_root in my case or not. As you can see from the code snippet in the initial report, I have a string and an union holding a table. I'll try to rewrite the whole thing with start/end and see if that helps. Any particular example I can to follow for doing this?

Looking at the buffer hex, I doublt they contain a file identifier, or if they do, C++ also has it, and then the python problem shoud be the same there.

File identifier is in both buffers; it is the "f142" string.

hinxx commented 2 years ago

I've fed the flatcc binary buffer into C++ verifier. It also fails.

Here is the C++ buffer again, after the hexdump the verification is made. No errors. Then I get the root object from the the binary buffer and try to access individual table elements. No problems there, all match the inputs I gave when building the buffer.

fb size 112
0000:   1C 00 00 00 66 31 34 32   00 00 00 00 10 00 20 00    ....f142...... .
0010:   0C 00 07 00 10 00 14 00   08 00 0A 00 10 00 00 00    ................
0020:   00 00 00 0A 03 00 01 00   14 00 00 00 38 00 00 00    ............8...
0030:   4E 61 BC 00 00 00 00 00   00 00 00 00 1A 00 00 00    Na..............
0040:   4C 41 42 3A 64 65 74 31   30 3A 41 72 72 61 79 43    LAB:det10:ArrayC
0050:   6F 75 6E 74 65 72 5F 52   42 56 00 00 00 00 06 00    ounter_RBV......
0060:   0C 00 04 00 06 00 00 00   00 00 00 00 00 00 F0 3F    ...............?
CPP source name LAB:det10:ArrayCounter_RBV
CPP value 1.000000
CPP timestamp 12345678
CPP severity MAJOR
CPP status HIHI

C binary buffer was written to file, and then loaded into a buffer for use with C++ verifier in the exact same manner as the C++ generated buffer above. The call to

    flatbuffers::Verifier verifier(buf, size);
    bool ret = VerifyLogDataBuffer(verifier);

fails and this is printed right after the hexdump. When looking at the individual elements it is interesting to see that except for the Value elements, all look correct. The Value element shows 0.000000 while it should be 1.000000.

fb size 110
0000:   0C 00 00 00 66 31 34 32   00 00 00 00 AE FF FF FF    ....f142........
0010:   4E 61 BC 00 00 00 00 00   20 00 00 00 3C 00 00 00    Na...... ...<...
0020:   03 00 01 00 0A 00 00 00   00 00 00 00 D4 FF FF FF    ................
0030:   00 00 00 00 00 00 F0 3F   1A 00 00 00 4C 41 42 3A    .......?....LAB:
0040:   64 65 74 31 30 3A 41 72   72 61 79 43 6F 75 6E 74    det10:ArrayCount
0050:   65 72 5F 52 42 56 00 00   06 00 0C 00 04 00 10 00    er_RBV..........
0060:   19 00 0C 00 18 00 10 00   04 00 14 00 16 00          ..............  
C buffer verification failed!
C source name LAB:det10:ArrayCounter_RBV
C value 0.000000
C timestamp 12345678
C severity MAJOR
C status HIHI

It is had to tell which bytes are the double value due to IEEE encoding and it might be better to try with uint64_t instead, to see where the value is and where the flatcc is looking for instead.

hinxx commented 2 years ago

Here are the results with using int64_t instead of double for my value element. I used 0x1122334455667788 for the value. I also used 0x1234567812345678 for the timestamp element. Now it is easy to spot them in the hexdumps.

C++:

fb size 112
0000:   1C 00 00 00 66 31 34 32   00 00 00 00 10 00 20 00    ....f142...... .
0010:   0C 00 07 00 10 00 14 00   08 00 0A 00 10 00 00 00    ................
0020:   00 00 00 07 03 00 01 00   14 00 00 00 38 00 00 00    ............8...
0030:   78 56 34 12 78 56 34 12   00 00 00 00 1A 00 00 00    xV4.xV4.........
0040:   4C 41 42 3A 64 65 74 31   30 3A 41 72 72 61 79 43    LAB:det10:ArrayC
0050:   6F 75 6E 74 65 72 5F 52   42 56 00 00 00 00 06 00    ounter_RBV......
0060:   0C 00 04 00 06 00 00 00   88 77 66 55 44 33 22 11    .........wfUD3".
CPP source name LAB:det10:ArrayCounter_RBV
CPP value 0x1122334455667788
CPP timestamp 0x1234567812345678
CPP severity MAJOR
CPP status HIHI

C:

fb size 110
0000:   0C 00 00 00 66 31 34 32   00 00 00 00 AE FF FF FF    ....f142........
0010:   78 56 34 12 78 56 34 12   20 00 00 00 3C 00 00 00    xV4.xV4. ...<...
0020:   03 00 01 00 07 00 00 00   00 00 00 00 D4 FF FF FF    ................
0030:   88 77 66 55 44 33 22 11   1A 00 00 00 4C 41 42 3A    .wfUD3".....LAB:
0040:   64 65 74 31 30 3A 41 72   72 61 79 43 6F 75 6E 74    det10:ArrayCount
0050:   65 72 5F 52 42 56 00 00   06 00 0C 00 04 00 10 00    er_RBV..........
0060:   19 00 0C 00 18 00 10 00   04 00 14 00 16 00          ..............  
C buffer verification failed!
C source name LAB:det10:ArrayCounter_RBV
C value 0x0
C timestamp 0x1234567812345678
C severity MAJOR
C status HIHI
hinxx commented 2 years ago

If I build the buffer using start/end it works as expected!


    LogData_start_as_root(B);

    Long_ref_t val = LogData_value_Long_create(B, 0x1122334455667788);
    Value_union_ref_t un = Value_as_Long(val);

    LogData_source_name_create_str(B, "LAB:det10:ArrayCounter_RBV");
    LogData_timestamp_add(B, 0x1234567812345678);
    LogData_value_add_value(B, un);
    LogData_value_add_type(B, un.type);
    LogData_severity_add(B, AlarmSeverity_MAJOR);
    LogData_status_add(B, AlarmStatus_HIHI);
    LogData_end_as_root(B);

The hexdump now looks like this:

fb size 110
0000:   0C 00 00 00 66 31 34 32   00 00 00 00 AE FF FF FF    ....f142........
0010:   07 00 00 00 38 00 00 00   14 00 00 00 00 00 00 00    ....8...........
0020:   78 56 34 12 78 56 34 12   01 00 03 00 1A 00 00 00    xV4.xV4.........
0030:   4C 41 42 3A 64 65 74 31   30 3A 41 72 72 61 79 43    LAB:det10:ArrayC
0040:   6F 75 6E 74 65 72 5F 52   42 56 00 00 F4 FF FF FF    ounter_RBV......
0050:   88 77 66 55 44 33 22 11   06 00 0C 00 04 00 10 00    .wfUD3".........
0060:   20 00 0C 00 04 00 08 00   14 00 1E 00 1C 00           .............  
mikkelfj commented 2 years ago

OK, technically what you did initially is wrong, but I thought at worked anyway at root level.

Some flatcc calls combine multiple operations like start/end bufer, start/end table as root, etc. Strings and other objects stored outside of tables must be created after the start call. This makes it hard to use in the create_as_root call. The buffer is not in a proper state before the start call and therefore it is undefined what happens when creating a string etc. early. However, I did look at a few cases in the past where it did some to work OK. But if you only can get it to work with start / end calls, then that assumption is likely wrong.

Note that there is not (or should not) be any problem with creating strings either before or after a table start call. It is only the buffer start that is a concern. But often these two calls are conflated in the _as_root call.

I ought to debug this further to understand, but my hands are a bit full ATM, and I realize there might be some documentation that could be improved.

Just to summarize your above postings: You do have it working for your needs now?

mikkelfj commented 2 years ago

Maybe a buffer context struct zeroing, or a buffer init call first. I do not recall, but that could be it. Though either you should always call init first, or, create as root or start as root does it and overwrites. I think you are missing an init call. See monster test for more details.

mikkelfj commented 2 years ago

Reading some of your comments more closely I see you have a case that fails although it is wrapped in buffer_start/stop. That is strange. The init call I was referring to is: https://github.com/dvidelabs/flatcc/blob/bca3b5fcd77c3e990fef75fae615b8f62f66af0e/test/monster_test/monster_test.c#L241

mikkelfj commented 2 years ago

Actually it is https://github.com/dvidelabs/flatcc/blob/bca3b5fcd77c3e990fef75fae615b8f62f66af0e/test/monster_test/monster_test.c#L2754 But you can use reset instead when building a second buffer.

mikkelfj commented 2 years ago

Assuming you have initialized your buffer correctly, it appears the difference is in the LogData_create call.

You can try to compare your manual code that works with the generated code for LogData_create. One difference is that it will probably use a generic create_str call and add the ref, but that should effectively be the same as you do.

hinxx commented 2 years ago

Just to summarize your above postings: You do have it working for your needs now?

Yeah. It works OK now with start/end.

I had the flatcc_builder_init() all this time.

I can report that this works:

    LogData_start_as_root(B);

    Long_ref_t val = LogData_value_Long_create(B, 0x1122334455667788);
    Value_union_ref_t un = Value_as_Long(val);
...
    LogData_end_as_root(B);

while this does not (resembles the initial code as there was no call to LogData_start_as_root()):

    Long_ref_t val = LogData_value_Long_create(B, 0x1122334455667788);
    Value_union_ref_t un = Value_as_Long(val);

    LogData_start_as_root(B);
...
    LogData_end_as_root(B);

FWIW, we can close this one. Thank you for the help!

mikkelfj commented 2 years ago

I'm a bit concerned with what you wrote here:

If was playing around with flatbuffers_buffer_start_aligned() like so:

flatbuffers_buffer_start_aligned(B, LogData_file_identifier, 4);
flatbuffers_string_ref_t pv = flatbuffers_string_create_str(B, vame);
Double_ref_t val = LogData_value_Double_create(B, 1.0);
Value_union_ref_t un = Value_as_Double(val);
logData = LogData_create(B, pv, un, 12345678, AlarmStatus_HIHI, AlarmSeverity_MAJOR);
flatbuffers_buffer_end(B, logData);

I'm not sure why that doesn't work. You can create objects before the table is started, and indeed this is the only way to do it in C++. It's just that doing it before the buffer is started is undefined. So I'd like to understand why this fails.

mikkelfj commented 2 years ago

You should use flatbuffers_buffer_end_aligned for symmetri, but I don't think that is the problem.

jobol commented 2 years ago

probably related to #127

mikkelfj commented 11 months ago

It took me while to get around this. If you build the code in debug mode and executes it, it becomes immediately apparent that the line Double_ref_t val = LogData_value_Double_create(B, 1.0); complains about the build stack not being of table type. It is more confusing why that is.

But it is actually correct:

The call works on the table LogData which has the field named value, and this value has type Double which is a table. So the call attempts to create a child table and add it to the parent table LogData, but the LogData table has not been started yet. This the fix is to start the LogData table first instead of using create later on, or alternatively create the Double table in isolation via Double_create instead of LogData_value_Double_create which is a convenience function we do not need here.

So overall it works as designed and I am removing the bug tag again.

Note that my above comment about flatbuffers_buffer_end_aligned is incorrect. There is no such function. It is common in flatcc to have an alias name for symmetrical operation, but not in this case, and it is documented as such in builder.md for better or worse.