Added protection to collection sizes with FC_ASSERTs #55
Conversation
|
IIRC we're not using |
| value.clear(); | ||
| FC_ASSERT( size.value*sizeof(T) < MAX_ARRAY_ALLOC_SIZE ); |
There was a problem hiding this comment.
This change is dangerous.
In the core, T is often an object_id_type with sizeof(T) = 8(?). MAX_ARRAY_ALLOC_SIZE/8 > MAX_NUM_ARRAY_ELEMENTS, so with this change we lose compatibility.
OTOH T is sometimes a public_key_type with sizeof(T) = 33(?). MAX_ARRAY_ALLOC_SIZE < MAX_NUM_ARRAY_ELEMENTS*33, which means we lose compatibility in the other direction.
(Note that the old check is stupid in general because sizeof(T) does not necessarily correlate with the actual storage size of a pack'd T.)
There was a problem hiding this comment.
I'd like to build a test to prove this, and I'd like to use an item where its size is not equal to its packed size. I haven't thought the process all the way through, but I'm thinking of a flat set of varint. Will that work? I will use small value varints for a baseline, and large value varints for the other way. I realize this does nothing for compatibility, but I believe it would help design asserts that actually check for what we want to prevent. Your thoughts please, @pmconrad.
There was a problem hiding this comment.
If you want to be realistic use object_id_type with instance values < 128.
Another example would be strings - sizeof(string) != string length.
| value.clear(); | ||
| FC_ASSERT( size.value*(sizeof(K)+sizeof(V)) < MAX_ARRAY_ALLOC_SIZE ); |
This is a PR for the bitshares-fc side of issue bitshares/bitshares-core#995
The #define was split in two to make these FC_ASSERTs easier.
Please note that these were not added to the pack/unpack methods for bip::vector<T,A>. I believe they should be, but want to wait for confirmation from others.