search

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

memcpy called with NULL as source pointer #45

Closed Pesa closed 5 years ago

Pesa commented 5 years ago

According to the C standard, it's undefined behavior. See https://www.imperialviolet.org/2016/06/26/nonnull.html for a longer explanation.

This is the first instance I found in the code, but there may be more:

https://github.com/named-data-iot/ndn-lite/blob/58a96e73293d488899c9cfc82caaba490705d792/util/msg-queue.c#L117

Here, param may be NULL (and is in fact NULL in the two invocations in forwarder/pit.c)

zjkmxy commented 5 years ago

Thanks for the link. Fixed now.

Pesa commented 5 years ago

Commit 49fd25ba397571a9b824153b8116d07fa8112a62 does not fix the issue. Calling memcpy with a NULL pointer is illegal even if the length is zero.

zjkmxy commented 5 years ago

Commit 49fd25b does not fix the issue. Calling memcpy with a NULL pointer is illegal even if the length is zero.

Now memcpy will only be called when the length > 0.

Pesa commented 5 years ago

Yes, and I'm saying that it's still wrong. memcpy(..., NULL, 0) is undefined behavior. Re-read the article I cited please.

zjkmxy commented 5 years ago

Yes, and I'm saying that it's still wrong. memcpy(..., NULL, 0) is undefined behavior. Re-read the article I cited please.

Yes. That's why I add an if to ensure when param_length == 0, the program will not call memcpy(ptail->param, NULL, 0). It's not allowed to call the function with param==NULL and param_length > 0. Given that this function is not an API, I don't need to check this case.

Pesa commented 5 years ago

oops I had a typo in my previous comment, sorry. I obviously meant memcpy(..., NULL, >0).

It's not allowed to call the function with param==NULL and param_length > 0. Given that this function is not an API, I don't need to check this case.

Well... ok, fair enough. It seems a simple check to add to guard against potential programming errors, but if you say calling code guarantees that condition cannot happen, that's fine with me.