[GitHub] [tvm] cxcxcxcx commented on a change in pull request #7190: Makes sure g_last_error is null terminated.

[GitBox <gi...@apache.org>]

cxcxcxcx commented on a change in pull request #7190:
URL: https://github.com/apache/tvm/pull/7190#discussion_r551159043



##########
File path: src/runtime/crt/common/crt_runtime_api.c
##########
@@ -38,7 +38,10 @@
 
 static char g_last_error[1024];
 
-void TVMAPISetLastError(const char* msg) { strncpy(g_last_error, msg, sizeof(g_last_error)); }
+void TVMAPISetLastError(const char* msg) {
+  strncpy(g_last_error, msg, sizeof(g_last_error) - 1);
+  g_last_error[sizeof(g_last_error) - 1] = 0;

Review comment:
       Does "truncate msg to the size of g_last_error" mean mutating "msg"? That doesn't sound always doable. My understanding:
   
   1. Before the proposed change, when the error message is longer than 1024, g_last_error won't be null terminated. I believe this is what the warning is about.
   2. With the change, g_last_error will always be null-terminated.
   3. If we use `strcpy`, we need to strlen first. If the length is long, we need to do something special. I don't think it's trivial. It could be faster, but I wonder how much it matters, since errors are probably rare.
   4. snprintf may be an alternative. But probably not too different in the context.
   
   Given these, would you reconsider the change as is?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org