[dbus] add API to fetch netdiag TLVs#3149
Conversation
Summary of ChangesHello @jdswensen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the OpenThread Border Router (OTBR) by adding a new D-Bus API for retrieving network diagnostic Type-Length-Value (TLV) data. This allows external applications to query detailed diagnostic information from the Thread network in a flexible and future-proof manner, aligning with existing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Example command: And reply: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new D-Bus API for fetching network diagnostic TLVs, which is a valuable addition. The implementation is mostly well-structured, but there's a critical logic issue in how asynchronous responses are handled. The current implementation only processes the first response and ignores any subsequent ones, which would not work correctly in a network with multiple devices. I've provided a detailed comment and a code suggestion to fix this behavior. Additionally, there are a couple of minor suggestions to improve code clarity and consistency.
| void ThreadHelper::NetDiagCallback(otError aError, otMessage *aMessage, const otMessageInfo *aMessageInfo) | ||
| { | ||
| otError error = OT_ERROR_NONE; | ||
| const uint16_t replySize = 1024; | ||
| uint8_t reply[replySize] = {}; | ||
| uint16_t written = 0; | ||
|
|
||
| OTBR_UNUSED_VARIABLE(aMessageInfo); | ||
| VerifyOrExit(mDiagHandler); | ||
|
|
||
| if (aError != OT_ERROR_NONE) | ||
| { | ||
| mDiagHandler(aError, {}); | ||
| } | ||
| else | ||
| { | ||
| SuccessOrExit(error = otThreadGetRawDiagnosticTlvs(aMessage, replySize, reply, &written)); | ||
| mNetworkDiagTlvResults.insert(mNetworkDiagTlvResults.end(), reply, reply + written); | ||
| mDiagHandler(error, mNetworkDiagTlvResults); | ||
| } | ||
|
|
||
| exit: | ||
| if (error != OT_ERROR_NONE && mDiagHandler) | ||
| { | ||
| mDiagHandler(error, {}); | ||
| } | ||
|
|
||
| mDiagHandler = nullptr; | ||
| } |
There was a problem hiding this comment.
The current implementation of NetDiagCallback incorrectly handles responses. It calls the handler and clears it upon receiving the first diagnostic response, which means any subsequent responses from other devices will be ignored. The otThreadSendDiagnosticGet function can trigger multiple callbacks (one per responding device) before a final timeout callback.
The correct approach is to accumulate results from all successful responses and only invoke the final handler mDiagHandler when the transaction is complete, which is typically signaled by a timeout (aError != OT_ERROR_NONE).
Additionally, allocating a 1KB buffer on the stack (uint8_t reply[replySize]) is risky and could lead to stack overflow. Using std::vector is a safer alternative.
The suggested fix below addresses both of these issues.
void ThreadHelper::NetDiagCallback(otError aError, otMessage *aMessage, const otMessageInfo *aMessageInfo)
{
otError error = OT_ERROR_NONE;
const uint16_t replySize = 1024;
std::vector<uint8_t> reply(replySize);
uint16_t written = 0;
OTBR_UNUSED_VARIABLE(aMessageInfo);
VerifyOrExit(mDiagHandler);
if (aError == OT_ERROR_NONE)
{
SuccessOrExit(error = otThreadGetRawDiagnosticTlvs(aMessage, reply.size(), reply.data(), &written));
reply.resize(written);
mNetworkDiagTlvResults.insert(mNetworkDiagTlvResults.end(), reply.begin(), reply.end());
// Wait for more responses or timeout.
ExitNow();
}
// End of transaction (timeout or other error), reply with what we have.
mDiagHandler(mNetworkDiagTlvResults.empty() ? aError : OT_ERROR_NONE, mNetworkDiagTlvResults);
mDiagHandler = nullptr;
exit:
if (error != OT_ERROR_NONE)
{
if (mDiagHandler)
{
mDiagHandler(error, {});
mDiagHandler = nullptr;
}
}
}There was a problem hiding this comment.
Is there a mechanism for defining timeouts within the dbus architecture? I think that might be required as an input if the caller wants to define how long to wait for responses.
For now, I refactored things so the dbus API will reject multicast addresses.
jdswensen
force-pushed
the
jds/dbus-netdiag-get
branch
from
433ae3d to
bf49e25
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new D-Bus API to fetch network diagnostic TLVs, which is a valuable addition. However, the current implementation has a few critical issues that need to be addressed. There's a use-after-free vulnerability due to incorrect handling of a buffer's lifetime in an asynchronous call, and a buffer over-read when processing the response message. Additionally, there's a logic error where a handler is not cleared in an error path, which could render the feature unusable after the first failure. I've provided detailed comments and suggestions to fix these problems.
Add an API to fetch arbitrary network diagnostic TLVs. This new API mirrors the `ot-ctl` command `networkdiagnostic get`. A generic approach was preferred over adding individual APIs so that an external application can support fetching future TLVs that OTBR may not support.
jdswensen
force-pushed
the
jds/dbus-netdiag-get
branch
from
bf49e25 to
d2f43b6
Compare
1 participant
Add an API to fetch arbitrary network diagnostic TLVs. This new API mirrors the
ot-ctlcommandnetworkdiagnostic get.A generic approach was preferred over adding individual APIs so that an external application can support fetching future TLVs that OTBR may not support.
Depends on: openthread/openthread#12191