Some asserts on oni_read_frame should be replaced by normal sanity checks

[aacuevas]

There are a couple of asserts on oni_read_frame() that sanity-check the header values before accessing memory. Specifically the ones that check data_sz to be >0 and <= max_frame_size. The issue with asserts is that they only work on debug mode, with release mode removing them.

While this was the original intention, as normal operation should always meet these sanity checks, so they are there for development purposes, communication or buffering issues could cause malformed frames which return invalid values for these fields which end causing memory access problems in subsequent memcpy calls. Having these asserts be normal checks could prevent crashes and notify users of issues on the communication backend that could trigger actions to help debug the issue.

[jonnew]

Of course I agree that it would be nice to have these checks. Are we worried about performance at all? This is the most performance critical call in the library by a wide margin. I guess a simple if statement is not going to be an issue in the grand scheme of things (e.g. in comparison to what needs to be done to data after it is collected).

[aacuevas]

Performance is why I discarded checking that the reported frame size equals to the expected frame size for that device, as that would mean looking through the hashtable, which is expensive.

But just checking against two constants (>0 && <= max_frame_size ) should be negligible in comparison with the mallocs that we are already doing there.

[jonnew]

Yeah, OK. Lets go ahead and add it then.

[jonnew]

Take care of this line (check if its even a valid statement first) and then we can merge.

liboni/api/liboni/oni.c

Line 671 in 8f40cb6

// TODO: max_read_frame_size contains the header as well so the upper bound