dvidelabs / flatcc

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

fix potential memory leaks in fileio.c and semantics.c #199

Closed Adsun701 closed 3 years ago

Adsun701 commented 3 years ago

This fixes potential memory leaks that were detected by a quick run of cppcheck. In fileio.c, memcpy() will only copy from prefix to path if prefix_len is greater than 0, so copying from a potentially null pointer is avoided. In semantics.c, index.len is set accordingly based on index.type so that the former variable will always be initialized.

mikkelfj commented 3 years ago

Copy operation: yes, that is technically undefined behaviour for null pointers regardless of length. Fine to fix though it hardly matters in praxis.

index: I don't recall what the len field is used for - but is not used with coerce. I'm not sure if it semantically makes sense to assing a length for numerics if the length field is not set elsewhere for numerics.

However, the index value ought to be zero initialized.

Adsun701 commented 3 years ago

@mikkelfj Fixed with index.len set to 0.

mikkelfj commented 3 years ago

I just researched in parser.c. It seems that the v->len is only used with type information where scalar types have length 1 and fixed length arrays can have other lengths. For normal integer parsing, the length field is not used and most likely defaults to zero. I think the length field was added relatively late to handle fixed length arrays.

Thus, your latest change is fine, but I think leaving the code as it were is more correct if you initialize with fb_value_t index = { 0 };.

Adsun701 commented 3 years ago

@mikkelfj Fixed, thanks for the input.

mikkelfj commented 3 years ago

Thanks