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

HCICardioTransport - RingBuffer _rxBuf may not be safe #318

Open
KurtE opened this issue Aug 9, 2023 · 0 comments
Open

HCICardioTransport - RingBuffer _rxBuf may not be safe #318

KurtE opened this issue Aug 9, 2023 · 0 comments
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project

Comments

@KurtE
Copy link

KurtE commented Aug 9, 2023

I was trying to build one of the ArduinoBLE examples (scan), using my updated core code for UNOR4 Wifi,
and: HCIVirtualTransport.cpp failed to compile as RingBufferN was not defined... This was with my current version I removed the usage of it from the Serial code.
Fix for this one is to explicitly include it in the file:
i.e. to add the line: #include "api/RingBuffer.h"

While I was looking at how the RingBuffer code was used in this library, I believe that the usage in
the HCICardioTransport class may be unsafe.

That is the buf.read_char is called from its main read method.

int HCICordioTransportClass::read()
{
  return _rxBuf.read_char();
}

And the store_char is called in the method HandleRxData which is called by
the method onDataReceived, which appears to be called by an event.

As I mentioned in the issue:
arduino/ArduinoCore-API#195
Both the read_char and store_char both modify a member variable: _numElems
To either increment or decrement it.

The problem is there is a timing window in the current code, Where if the callback happens while in the read_char, right after it reads in the current value of _numElems and before it stores the updated count back out, the callback happens, which updates the count. When it returns, the storing out of the updated count in the read_char will wipeout the count changes that happened in the store_char call(s) - could be be multiple bytes that are added during the callback.

Hope this was clear.

@per1234 per1234 added type: imperfection Perceived defect in any part of project topic: code Related to content of the project itself labels Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

No branches or pull requests

2 participants