Re: [RFC PATCH v6 03/11] trace-cmd: Add TRACE_REQ and TRACE_RESP messages

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 14 Feb 2019 16:13:27 +0200
Slavomir Kaslev <kaslevs@vmware.com> wrote:

> + /*
> +  * NOTE: On success, the returned `argv` should be freed with:
> +  *     free(argv[0]);
> +  *     free(argv);
> +  */
> +int tracecmd_msg_recv_trace_req(struct tracecmd_msg_handle *msg_handle,
> +				int *argc, char ***argv)
> +{
> +	struct tracecmd_msg msg;
> +	char *p, *buf_end, **args;
> +	int i, ret, nr_args;
> +	ssize_t buf_len;
> +
> +	ret = tracecmd_msg_recv(msg_handle->fd, &msg);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ntohl(msg.hdr.cmd) != MSG_TRACE_REQ) {
> +		ret = -ENOTSUP;
> +		goto out;
> +	}
> +
> +	nr_args = ntohl(msg.trace_req.argc);
> +	if (nr_args <= 0) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	buf_len = ntohl(msg.hdr.size) - MSG_HDR_LEN - ntohl(msg.hdr.cmd_size);
> +	buf_end = (char *)msg.buf + buf_len;
> +	if (buf_len <= 0 && ((char *)msg.buf)[buf_len-1] != '\0') {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	args = calloc(nr_args, sizeof(*args));
> +	if (!args) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	p = msg.buf;
> +	for (i = 0; i < nr_args; i++) {
> +		if (p >= buf_end) {
> +			ret = -EINVAL;
> +			goto out_args;
> +		}
> +		args[i] = p;
> +		p = strchr(p, '\0');
> +		if (!p) {
> +			ret = -EINVAL;
> +			goto out_args;
> +		}
> +		p++;
> +	}
> +
> +	/*
> +	 * On success we're passing msg.buf to the caller through argv[0] so we
> +	 * don't msg_free in this case.
> +	 */

This is OK for now, but I'm still wondering if we should just add:

	msg.buf = NULL;
	msg_free(&msg);

For robustness. What happens if for some reason in the future that we
add another allocation to msg? That would cause this to leak memory.

Yeah, yeah, this is highly paranoid, but code like this is exactly how
memory leaks come about and other bugs come about. It's taking advantage
of internals of how msg_free() works to optimize the case. And when the
internals change, things break.

No need to change anything here, I may just add the code on top of this
series.

-- Steve


> +	*argc = nr_args;
> +	*argv = args;
> +	return 0;
> +
> +out_args:
> +	free(args);
> +out:
> +	error_operation(&msg);
> +	if (ret == -EOPNOTSUPP)
> +		handle_unexpected_msg(msg_handle, &msg);
> +	msg_free(&msg);
> +	return ret;
> +}
> +