named-data-iot/ndn-lite

memcpy called with NULL as source pointer

Closed this issue · 6 comments

Pesa commented

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:

memcpy(ptail->param, param, param_length);

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

Thanks for the link. Fixed now.

Pesa commented

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

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

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

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

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.