-
Notifications
You must be signed in to change notification settings - Fork 95
Conversation
vmdk_plugin/sanity_test.go
Outdated
// } | ||
// }(idx, clients[0].client) | ||
// } | ||
//TODO: Temporarily disable test until #1062 is root caused & fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this comment.
esx_service/vmci/vmci_client.c
Outdated
@@ -360,7 +360,7 @@ Vmci_GetReply(int port, const char* json_request, const char* be_name, | |||
return CONN_FAILURE; | |||
} | |||
|
|||
req.mlen = strnlen(json_request, MAXBUF) + 1; | |||
req.mlen = strnlen(json_request, MAXBUF-1) + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great this fixes the issue but I am not sure if I understand the fix. Original code had "+1" to accomodate terminating null byte. Does it mean either c-go or vmci has issues when buffers crosses 1M boundry and another way to fix is make MAXBUF = (1024 * 1024) - 1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@govint ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry missed updating, the original code was creating a string which was MAXBUF + 1, the strnlen() can create a string that can be at most MAXBUF length in size and over that it adds the NULL byte. The change is to create a string thats MAXBUF -1 (1MB -1) plus the NULL byte which makes a total of MAXBUF. We may never have a request size of 1MB but the code is not correct as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still puzzled how this change automagically fixes crash in concurrency test. Did we have a case where req.mlen is (MAXBUF+1)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix is from reviewing the code, the original panic isn't reproducible anymore with the GO env. upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@govint - thanks for pointing to this. While this +- issue could not have been a reason for test failing (see below) , the buffer size control needs fixing, albeit in a separate PR.
Here is the summary :
the original code was creating a string which was MAXBUF + 1,
Actually it was not - the string was created in GO with whatever size, and the code tried to make sure that strings > 1M were truncated before being passed to VMCI but the actual truncation is missing (indeed a bug).
However, this was never hit, because vmdk_opsd.py MAX_JSON_SIZE is set to 4K, and for vmci message > 4K the vmci_server.c receive() code will reject the message and drop the connection with "message too large" , without even trying to receive the message body !
The bottom line:
- Please remove this line from the PR and simply enable the test. LGTM from me on that
- I also suggest a separate PR that cleans up the inconsistency. I suggest making MAXBUF consistent with MAX_JSON_SIZE, put comments in the code with cross-reference, and then limit strings to MAXBUF-1 plus NULL termination -
In this new PR this specific text would change to
req.mlen = strnlen(json_request, MAXBUF - 1) + 1;
req.msg = json_request; // sizeof string, with null temintation
req.msg[req.mlen] = NULL; // just in case we hit the MAXBUF limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, I was just responding on the string sizing logic of what I'd made.
Retain calloc of response buffer, verify original issue is reproduced. Minor fix in vmci_client.c code where the request sent is kept within MAXBUF size limit.
The build is successful and original panic isn't reproduced. Most likely it may be a GO/C run time issue that was addressed with the recent upgrade of Go run time.