Safer use of strncpy patch #578
Conversation
of strncpy() in the open-amp project. In one place optional {...} braces
were added to an else branch because its if branch now needs braces.
Signed-off-by: David Adams <david@tarma.com>
| @@ -142,6 +142,7 @@ int rpmsg_send_ns_message(struct rpmsg_endpoint *ept, unsigned long flags) | |||
| ns_msg.flags = flags; | |||
| ns_msg.addr = ept->addr; | |||
| strncpy(ns_msg.name, ept->name, sizeof(ns_msg.name)); | |||
| ns_msg.name[sizeof(ns_msg.name) - 1] = '\0'; | |||
There was a problem hiding this comment.
this one seems useless as the '\0' has already been addes in rpmsg_register_endpoint
There was a problem hiding this comment.
@arnopo : No -- that would be unwise, because it relies on ept->name only being set by rpmsg_register_endpoint. It's just a micro-optimization to remove the '\0' addition here, because at the next code review people might wonder why this strncpy wasn't updated to a safer version. Best to leave it.
There was a problem hiding this comment.
A comment should also address your concern without adding useless code. "Moreover, this '\0' addition can give the impression that the endpoint name could be truncated."
For instance:
/*The strncpy is safe at this point. The '\0' has been added in rpmsg_register_endpoint(). */
There was a problem hiding this comment.
Thanks, but I don't want to submit a compromised patch. I have withdrawn the PR and suggest that you create your own patch if you thinks it's important enough.
There was a problem hiding this comment.
@arnopo: Additional:
(1) If you use strncpy(), always follow it with explicit \0 termination. No exceptions, no special pleading.
(2) If you known that source and destination buffers are the same size and the the source is already 0-terminated, then use memcpy() instead of strncpy()
I suggest that you make those changes yourself. I'm not your little helper.
There was a problem hiding this comment.
@arnopo: Additional:
(1) If you use strncpy(), always follow it with explicit \0 termination. No exceptions, no special pleading.
(2) If you known that source and destination buffers are the same size and the the source is already 0-terminated, then use memcpy() instead of strncpy()
I suggest that you make those changes yourself. I'm not your little helper.
Please keepa positive mindset. This is an open-source project based on contributions. Contributors are not 'little helpers' for the maintainers, and maintainers are not subcontractors assigned to the project. If a user finds an issue, it seems fair for them to propose a fix as a contributor to help improve the library. We review and comment/challenge when something seems to need improvement. If contributors do not agree with a comment, we discuss and try to find a solution. That's just the normal process.
That said, your proposal for the memcpy() seems to me a good compromise.
There was a problem hiding this comment.
Please keepa positive mindset. This is an open-source project based on contributions. Contributors are not 'little helpers' for the maintainers, and maintainers are not subcontractors assigned to the project. If a user finds an issue, it seems fair for them to propose a fix as a contributor to help improve the library. We review and comment/challenge when something seems to need improvement. If contributors do not agree with a comment, we discuss and try to find a solution. That's just the normal process.
Thanks @arnopo for clearing this point.
I was also looking another option to use strlcpy as safer replacement of strncpy.
https://linux.die.net/man/3/strlcpy
Thanks.
|
LGTM after addressing Arnaud's concern. |
@arnopo : as requested
This patch adds 0-termination of the destination buffer after each call of strncpy() in the open-amp project. In one place optional {...} braces were added to an else branch because its if branch now needs braces.