dark0707
/
px4-firmware
forked from Archive/PX4-Autopilot
1
0
Fork 0
Code Pull Requests Activity

uORB: fix node_open: *instance is read even though it's an output parameter 

This fixes a subtle bug: the instance parameter of orb_advertise is an output
parameter and thus its value can be random. However in node_open this value
is accessed and thus the open(...) call could succeed even though it should
not. This can happen for example if a second advertiser of a topic calls
orb_advertise_multi with *instance=0.

The existing implementation worked only because *instance was initialized
with -1 in most cases.
This commit is contained in:
Beat Küng 2016-04-15 12:46:49 +02:00 committed by tumbili
parent 995710da00
commit 9c360e9f88
4 changed files with 33 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/modules/uORB/uORBMain.cpp
View File

@ -94,6 +94,10 @@ uorb_main(int argc, char *argv[])
* Test the driver/device.
*/
if (!strcmp(argv[1], "test")) {
if (!g_dev) {
PX4_WARN("orb is not running! start it first");
return -ESRCH;
}
uORBTest::UnitTest &t = uORBTest::UnitTest::instance();
return t.test();
}

32
src/modules/uORB/uORBManager_nuttx.cpp
View File

@ -225,7 +225,7 @@ int uORB::Manager::node_open
)
{
char path[orb_maxpath];
int fd, ret;
int fd = -1, ret;
/*
* If meta is null, the object was not defined, i.e. it is not
@ -244,25 +244,21 @@ int uORB::Manager::node_open
return ERROR;
}
/*
* Generate the path to the node and try to open it.
*/
ret = uORB::Utils::node_mkpath(path, f, meta, instance);
/* if we have an instance and are an advertiser, we will generate a new node and set the instance,
* so we do not need to open here */
if (!instance || !advertiser) {
/*
* Generate the path to the node and try to open it.
*/
ret = uORB::Utils::node_mkpath(path, f, meta, instance);
if (ret != OK) {
errno = -ret;
return ERROR;
}
if (ret != OK) {
errno = -ret;
return ERROR;
}
/* open the path as either the advertiser or the subscriber */
fd = open(path, (advertiser) ? O_WRONLY : O_RDONLY);
/* if we want to advertise and the node existed, we have to re-try again */
if ((fd >= 0) && (instance != nullptr) && (advertiser)) {
/* close the fd, we want a new one */
close(fd);
/* the node_advertise call will automatically go for the next free entry */
fd = -1;
/* open the path as either the advertiser or the subscriber */
fd = open(path, advertiser ? O_WRONLY : O_RDONLY);
}
/* we may need to advertise the node... */

36
src/modules/uORB/uORBManager_posix.cpp
View File

@ -233,7 +233,7 @@ int uORB::Manager::node_open
)
{
char path[orb_maxpath];
int fd, ret;
int fd = -1, ret;
/*
* If meta is null, the object was not defined, i.e. it is not
@ -252,29 +252,21 @@ int uORB::Manager::node_open
return ERROR;
}
/*
* Generate the path to the node and try to open it.
*/
/* if we have an instance and are an advertiser, we will generate a new node and set the instance,
* so we do not need to open here */
if (!instance || !advertiser) {
/*
* Generate the path to the node and try to open it.
*/
ret = uORB::Utils::node_mkpath(path, f, meta, instance);
// FIXME - if *instance is uninitialized, why is this being called? Seems risky and
// its definiately a waste. This is the case in muli-topic test.
ret = uORB::Utils::node_mkpath(path, f, meta, instance);
if (ret != OK) {
errno = -ret;
return ERROR;
}
if (ret != PX4_OK) {
errno = -ret;
return ERROR;
}
/* open the path as either the advertiser or the subscriber */
fd = px4_open(path, (advertiser) ? PX4_F_WRONLY : PX4_F_RDONLY);
/* if we want to advertise and the node existed, we have to re-try again */
if ((fd >= 0) && (instance != nullptr) && (advertiser)) {
/* close the fd, we want a new one */
px4_close(fd);
/* the node_advertise call will automatically go for the next free entry */
fd = -1;
/* open the path as either the advertiser or the subscriber */
fd = px4_open(path, advertiser ? PX4_F_WRONLY : PX4_F_RDONLY);
}
/* we may need to advertise the node... */

2
src/modules/uORB/uORBTest_UnitTest.cpp
View File

@ -124,7 +124,7 @@ int uORBTest::UnitTest::pubsublatency_main(void)
delete[] timings;
warnx("mean: %8.4f", static_cast<double>(latency_integral / maxruns));
warnx("mean: %8.4f us", static_cast<double>(latency_integral / maxruns));
pubsubtest_passed = true;