Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cbpy: Fix error handling in get_sample_group #118

Merged
merged 1 commit into from
Nov 11, 2022

Conversation

stinos
Copy link
Contributor

@stinos stinos commented Feb 17, 2021

When cbSdkGetSampleGroupList returns an error the nChansInGroup variable
is in fact uninitialized so it can be any number, and usually very large.
As such looping over range(nChansInGroup) results in problems due to
undefined behavior, for example segfaults or a ZeroDivisionError when
dividing by digRange because it is unitialized for out-of-range channels.
Fix this by producing one single error when requesting a non-existing
sample group, and returning immediately in that case. Remove the extra
call for getting the number of channels since it's incorrect to test the
nChansInGroup variable.

When cbSdkGetSampleGroupList returns an error the nChansInGroup variable
is in fact uninitialized so it can be any number, and usually very large.
As such looping over range(nChansInGroup) results in problems due to
undefined behavior, for example segfaults or a ZeroDivisionError when
dividing by digRange because it is unitialized for out-of-range channels.
Fix this by producing one single error when requesting a non-existing
sample group, and returning immediately in that case. Remove the extra
call for getting the number of channels since it's incorrect to test the
nChansInGroup variable.
@cboulay
Copy link
Collaborator

cboulay commented Feb 17, 2021

You can see in the following snippet that the first call to cbSdkGetSampleGroupList (that you deleted) is treated differently than the second.
https://github.com/dashesy/CereLink/blob/3c076b48bee43ed35297bb726b5926f7b4df5776/cbhwlib/cbhwlib.cpp#L1345-L1351

But I can't find a practical reason for that, at least not in this instance. Typically with this idiom, the first call would give you the nChansInGroup which you could then use to initialize some other memory that would be passed to the second call. That doesn't seem to be the case here. Maybe the ability to call the function with null for the pGroupList is just for performance reasons, to avoid the memcpy if it isn't desired.

So are there any performance implications from removing the fail-fast first call? Probably not because the second call will still fail-fast in the same way the first would, right?
I'll need to find some time to think about it some more but if you are looking it over please check to see if there are any hidden implications to removing the first call to cbSdkGetSampleGroupList.

@stinos
Copy link
Contributor Author

stinos commented Feb 17, 2021

I didn't find hidden implications, and seeing the code path is straightforward I doubt there are any.
The impact now basically is

  • the path which gets used most (probably, assuming people usually want to know specs of existing sample groups) is marginally faster because only one call is needed
  • the fail path in the case where one is lucky and nChansInGroup is 0 is slower because there's an unneeded allocation of an unit16_t array of cbNUM_ANALOG_CHANS elements

@cboulay cboulay merged commit 00e366b into CerebusOSS:master Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants