named-data-iot / ndn-lite

A lightweight NDN protocol stack with high-level application support including security bootstrapping, access control, trust management, etc.
https://ndn-lite.named-data.net
GNU Lesser General Public License v3.0
44 stars 16 forks source link

ndn_msgqueue_dispatch: unaligned load #55

Open yoursunny opened 5 years ago

yoursunny commented 5 years ago

As of 097f568e0d55b60264f387c5d0b1770a5ce42ecf, 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 commented 5 years ago

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 commented 5 years ago

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.