search

open-ephys / liboni

API for controlling ONI-compliant hardware
https://open-ephys.github.io/onix-docs/API%20Reference/index.html
0 stars 5 forks source link

Some asserts on oni_read_frame should be replaced by normal sanity checks #15

Open aacuevas opened 11 months ago

aacuevas commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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