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.

Sign up for GitHub

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

Jump to bottom

Bugfix/parsing wrong vector yields segfault #1

Merged
merged 7 commits into from
Jan 6, 2023
Merged

Bugfix/parsing wrong vector yields segfault #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
18c3574
Switch to hpp headers of pluginlib (#230)
jspricke May 31, 2022
dbc73ff
Diagnostic meesage requires a string instead of bytes (#233)
chrisflesher Jul 12, 2022
d27f3ff
Fix diagnostics Updater constructor NodeHandle documentation (#234)
Roboterbastler Aug 1, 2022
4be469a
Fix Python3 port of hd_monitor.py (#231)
Rayman Oct 19, 2022
12ba2ee
Fix python3 shebang for diagnostic_analysis (#241)
SubaruArai Nov 15, 2022
cb25f04
Merge remote-tracking branch 'tracked_repo/noetic-devel' into nobleo-…
Jan 5, 2023
d02da08
Bugfix! We parse the wrong vector here which could yield a segfault
Jan 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion diagnostic_aggregator/src/aggregator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,8 @@ void Aggregator::publishData()
{
diag_array.status.push_back(*processed_other[i]);

const uint depth = static_cast<uint>(std::count(processed[i]->name.begin(), processed[i]->name.end(), '/'));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the actual fix, the rest is a sync with the forked remote repo

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH; I don't know what the implications of this change are down the line since I don't know the inner workings of this code. But I agree that this looks like a bug.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now something useful happens since we access elements from the vector we are actually looping through. Before we didn't and that could potentially throw a segfault...
So this change prevents a segfault and also makes sure we actually report something useful ;-)

const uint depth = static_cast<uint>(
std::count(processed_other[i]->name.begin(), processed_other[i]->name.end(), '/'));
if (processed_other[i]->level > diag_toplevel_state.level)
{
diag_toplevel_state.level = processed_other[i]->level;
Expand Down
2 changes: 1 addition & 1 deletion diagnostic_analysis/scripts/export_csv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/usr/bin/python
#!/usr/bin/python3
#
# Software License Agreement (BSD License)
#
Expand Down
2 changes: 1 addition & 1 deletion diagnostic_analysis/scripts/sparse_csv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/usr/bin/python
#!/usr/bin/python3
#
# Software License Agreement (BSD License)
#
Expand Down
2 changes: 1 addition & 1 deletion diagnostic_analysis/src/diagnostic_analysis/exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/usr/bin/python
#!/usr/bin/python3
#
# Software License Agreement (BSD License)
#
Expand Down
2 changes: 1 addition & 1 deletion diagnostic_analysis/src/diagnostic_analysis/sparse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/usr/bin/python
#!/usr/bin/python3
#
# Software License Agreement (BSD License)
#
Expand Down
6 changes: 3 additions & 3 deletions diagnostic_common_diagnostics/src/diagnostic_common_diagnostics/hd_monitor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,15 @@ def get_hddtemp_data(hostname='localhost', port=7634):
try:
hd_sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
hd_sock.connect((hostname, port))
sock_data = ''
sock_data = b''
while True:
newdat = hd_sock.recv(1024)
if len(newdat) == 0:
break
sock_data = sock_data + str(newdat)
sock_data = sock_data + newdat
hd_sock.close()

sock_vals = sock_data.split('|')
sock_vals = sock_data.decode().split('|')

# Format of output looks like ' | DRIVE | MAKE | TEMP | '
idx = 0
Expand Down
4 changes: 2 additions & 2 deletions diagnostic_common_diagnostics/src/diagnostic_common_diagnostics/ntp_monitor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ def ntp_diag(st, host, off, error_offset):
st.values = [ DIAG.KeyValue("Offset (us)", "N/A"),
DIAG.KeyValue("Offset tolerance (us)", str(off)),
DIAG.KeyValue("Offset tolerance (us) for Error", str(error_offset)),
DIAG.KeyValue("Output", o),
DIAG.KeyValue("Errors", e) ]
DIAG.KeyValue("Output", o.decode()),
DIAG.KeyValue("Errors", e.decode()) ]

return st

Expand Down
4 changes: 3 additions & 1 deletion diagnostic_updater/include/diagnostic_updater/diagnostic_updater.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,8 +368,10 @@ namespace diagnostic_updater
/**
* \brief Constructs an updater class.
*
* \param h Node handle from which to get the diagnostic_period
* \param h Node handle used to publish the diagnostics messages.
* \param ph Node handle from which to get the diagnostic_period
* parameter.
* \param node_name Name of the node used in the messages.
*/
Updater(ros::NodeHandle h = ros::NodeHandle(), ros::NodeHandle ph = ros::NodeHandle("~"), std::string node_name = ros::this_node::getName()) : private_node_handle_(ph), node_handle_(h), node_name_(node_name)
{
Expand Down
2 changes: 1 addition & 1 deletion test_diagnostic_aggregator/include/test_diagnostic_aggregator/fail_init_analyzer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
#include <diagnostic_aggregator/analyzer.h>
#include <diagnostic_aggregator/status_item.h>
#include <diagnostic_msgs/DiagnosticStatus.h>
#include <pluginlib/class_list_macros.h>
#include <pluginlib/class_list_macros.hpp>
#include <string>

namespace test_diagnostic_aggregator {
Expand Down
2 changes: 1 addition & 1 deletion test_diagnostic_aggregator/include/test_diagnostic_aggregator/match_no_analyze_analyzer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
#include <diagnostic_aggregator/analyzer.h>
#include <diagnostic_aggregator/status_item.h>
#include <diagnostic_msgs/DiagnosticStatus.h>
#include <pluginlib/class_list_macros.h>
#include <pluginlib/class_list_macros.hpp>
#include <string>

namespace test_diagnostic_aggregator {
Expand Down