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

ndn_msgqueue_dispatch: unaligned load #55

Open
yoursunny opened this issue Aug 19, 2019 · 2 comments
Open

ndn_msgqueue_dispatch: unaligned load #55

yoursunny opened this issue Aug 19, 2019 · 2 comments

Comments

@yoursunny
Copy link
Contributor

yoursunny commented Aug 19, 2019

As of 097f568, ndn_msgqueue_dispatch can potentially cause address error exception due to unaligned load when running on MIPS32 architecture.

The msgqueue operates on this data structure:

#pragma pack(1)
typedef struct ndn_msg{
  void* obj;
  ndn_msg_callback func;
  size_t length;
  uint8_t param[];
} ndn_msg_t;
#pragma pack()

ndn_msgqueue_post function stores instances of ndn_msg consecutively in the msg_queue buffer. If param_length is not a multiple of 4, the next ndn_msg struct becomes unaligned.
MIPS compiler will generate unaligned load/store instructions for ndn_msg structs themselves because of the pack(1) tag.
However, the param struct passed to the callback function would also be unaligned, and it's likely that that struct has not been declared as pack(1). Consequently, the callback function could perform a regular load instruction on an unaligned address, triggering an address error exception.

This issue has not caused a crash so far, because every invocation of ndn_msgqueue_post in current ndn-lite codebase has been setting param_length to zero. Upon this observations, a potential solution to this bug would be removing param and param_length params.

@zjkmxy
Copy link
Collaborator

zjkmxy commented Aug 19, 2019

Thank you for your report. I made param to be variable in length because I thought people may use this message to store a small Data packet. But currently no package is using msg_queue in this way because it's neither thread-safe nor reentrant. I would replace length and param with a single void* field if this causes problem in the future.

@yoursunny
Copy link
Contributor Author

I would replace length and param with a single void* field if this causes problem in the future.

Yes, this would be a good solution. Many C libraries handle arguments like this.
After that, #pragma pack(1) is no longer necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants