Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Handle potential invalid pointer reference for VMCI reply. #973

Closed
wants to merge 1 commit into from
Closed

Handle potential invalid pointer reference for VMCI reply. #973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions esx_service/vmci/vmci_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ vsock_get_reply(be_sock_id *s, be_request *r, be_answer* a)
ret = recv(s->sock_id, a->buf, b, 0);
if (ret == -1 || ret != b) {
free(a->buf);
a->buf = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - well, just to be paranoid, it should be

   if (a->buf) {
     free(a->buf);
     a->buf = NULL;
  }

I know a->buf is not null (per L286-L289, but just to keep this section easier to read and more resilient to changes above it :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return CONN_FAILURE;
}

Expand Down Expand Up @@ -349,5 +350,7 @@ Vmci_GetReply(int port, const char* json_request, const char* be_name,
req.mlen = strnlen(json_request, MAXBUF) + 1;
req.msg = json_request;

ans->buf = NULL;
Copy link
Contributor

@msterin msterin Feb 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, why here? if you want it to be NULL, the right place to change would be changing C.Malloc() to C.Calloc() in esx_vmdkcmd.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


return host_request(be, &req, ans, ESX_VMCI_CID, port);
}
5 changes: 5 additions & 0 deletions vmdk_plugin/drivers/vmdk/vmdkops/esx_vmdkcmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,11 @@ func (vmdkCmd EsxVmdkCmd) Run(cmd string, name string, opts map[string]string) (
break
}

if ans.buf == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do no think we can get here in failure case without printing ANY message from the loop above, and the logs did not have any message, so I do not see why this block is needed. Besides, with the changes suggested in this PR this can be impossible, will it ? Besides, in cgo is it ok to compare go.nil with C (unsafe) ans.buf ?

  • if you want to for working on a repro, this makes sense, but for production code it feels not needed

msg := "VMCI call to ESX failed, returned nil data."
log.Warnf(msg)
return nil, errors.New(msg)
}
response := []byte(C.GoString(ans.buf))
C.free(unsafe.Pointer(ans.buf))
err = unmarshalError(response)
Expand Down