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

core: add mavlink statustexts as debug printfs #119

Merged
merged 1 commit into from
Oct 23, 2017
Merged

core: add mavlink statustexts as debug printfs #119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
48 changes: 48 additions & 0 deletions core/device_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ DeviceImpl::DeviceImpl(DroneCoreImpl *parent,
register_mavlink_message_handler(
MAVLINK_MSG_ID_AUTOPILOT_VERSION,
std::bind(&DeviceImpl::process_autopilot_version, this, _1), this);

register_mavlink_message_handler(
MAVLINK_MSG_ID_STATUSTEXT,
std::bind(&DeviceImpl::process_statustext, this, _1), this);
}

DeviceImpl::~DeviceImpl()
Expand Down Expand Up @@ -155,6 +159,50 @@ void DeviceImpl::process_autopilot_version(const mavlink_message_t &message)
unregister_timeout_handler(_autopilot_version_timed_out_cookie);
}

void DeviceImpl::process_statustext(const mavlink_message_t &message)
{
mavlink_statustext_t statustext;
mavlink_msg_statustext_decode(&message, &statustext);

std::string debug_str = "mavlink ";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this enough to differentiate device as opposed to DroneCore originated warnings? Maybe text:
MAVLink: emergency or Device: emergency.
Might even be worth having a special class of debug for these. Just a thought.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed @hamishwillee, it would be good to understand when are these called and if we should be raising exceptions or alerting the users rather than just using them for debugging.

That said this PR is to have more debugging data, so we should merge and get someone to research onto what the messages mean, I want to know if we should be taking more critical actions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hamishwillee I can change it to MAVLink: emergency.

@mrpollo to catch errors, it might make sense that the plugins check for errors and inform the API user if it is relevant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mrpollo That said this PR is to have more debugging data, so we should merge and get someone to research onto what the messages mean, I want to know if we should be taking more critical actions

@mrpollo to catch errors, it might make sense that the plugins check for errors and inform the API user if it is relevant.

@hamishwillee I can change it to MAVLink: emergency.

Agree with all above - yes, ideally plugins should check for errors and work out what is relevant but we need to do more research on this. IMO we should do change suggested (MAVLink: xxxx) for now, accept PR, and raise issue to look at errors as a separate task.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, merge whenever. I have created #127 to add eventual task to look at these messages.


switch (statustext.severity) {
case MAV_SEVERITY_EMERGENCY:
debug_str += "emergency";
break;
case MAV_SEVERITY_ALERT:
debug_str += "alert";
break;
case MAV_SEVERITY_CRITICAL:
debug_str += "critical";
break;
case MAV_SEVERITY_ERROR:
debug_str += "error";
break;
case MAV_SEVERITY_WARNING:
debug_str += "warning";
break;
case MAV_SEVERITY_NOTICE:
debug_str += "notice";
break;
case MAV_SEVERITY_INFO:
debug_str += "info";
break;
case MAV_SEVERITY_DEBUG:
debug_str += "debug";
break;
default:
break;
}

// statustext.text is not null terminated, therefore we copy it first to
// an array big enough that is zeroed.
char text_with_null[sizeof(statustext.text) + 1] {};
memcpy(text_with_null, statustext.text, sizeof(statustext.text));
Copy link
Member

Choose a reason for hiding this comment

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

dirty indeed, but I don't see any other way around this if we can't control the input, and now that I think about it, can this lead to memory leaks? can someone use this to DOS an app using DroneCore by crafting big malicious messages?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think all that a malicious drone can do here is to print a bunch of memory that follows after statustext.text until 0 appears.

Copy link
Member

Choose a reason for hiding this comment

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

In conclusion, yes we make ourselves open to a crafty mavlink attack, but I'm sure this isn't the only place where this occurs


LogDebug() << debug_str << ": " << text_with_null;
}

void DeviceImpl::heartbeats_timed_out()
{
LogInfo() << "heartbeats timed out";
Expand Down
1 change: 1 addition & 0 deletions core/device_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ class DeviceImpl

void process_heartbeat(const mavlink_message_t &message);
void process_autopilot_version(const mavlink_message_t &message);
void process_statustext(const mavlink_message_t &message);
void heartbeats_timed_out();
void set_connected();
void set_disconnected();
Expand Down