Re: [PATCHv2 iproute2 1/2] lib/libnetlink: re malloc buff if size is not enough

Hi Michal,

On Mon, Sep 18, 2017 at 09:55:05AM +0200, Michal Kubecek wrote:

quoted

+static int rtnl_recvmsg(int fd, struct msghdr *msg, char **answer)
+{
+	struct iovec *iov;
+	int len = -1, buf_len = 32768;
+	char *bufp, *buf = NULL;
+
+	int flag = MSG_PEEK | MSG_TRUNC;
+
+realloc:
+	bufp = realloc(buf, buf_len);
+
+	if (bufp == NULL) {
+		fprintf(stderr, "malloc error: not enough buffer\n");
+		free(buf);
+		return -ENOMEM;
+	}
+	buf = bufp;
+	iov = msg->msg_iov;
+	iov->iov_base = buf;
+	iov->iov_len = buf_len;
+
+recv:
+	len = recvmsg(fd, msg, flag);
+
+	if (len < 0) {
+		if (errno == EINTR || errno == EAGAIN)
+			goto recv;
+		fprintf(stderr, "netlink receive error %s (%d)\n",
+			strerror(errno), errno);

free(buf);

quoted

+		return len;

Maybe we should return -errno (saved before calling fprintf()) to be
consistent.

quoted

+	}
+
+	if (len == 0) {
+		fprintf(stderr, "EOF on netlink\n");

free(buf);

Will fix these three issues.

quoted

@@ -471,19 +516,23 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
 
 				if (h->nlmsg_type == NLMSG_ERROR) {
 					rtnl_dump_error(rth, h);
+					free(buf);
 					return -1;
 				}
 
 				if (!rth->dump_fp) {
 					err = a->filter(&nladdr, h, a->arg1);
-					if (err < 0)
+					if (err < 0) {
+						free(buf);
 						return err;
+					}
 				}
 
 skip_it:
 				h = NLMSG_NEXT(h, msglen);
 			}
 		}
+		free(buf);

We only free the last buffer returned by rtnl_recvmsg() this way. IMHO
this free(buf) should be moved inside the loop.

Do you mean the outside while loop or the for loop? I think we could not put
it inside the for loop, because we may need the buf multi times based on arg.

	while (1) {
		status = rtnl_recvmsg(rth->fd, &msg, &buf);

		for (a = arg; a->filter; a++) {
			struct nlmsghdr *h = (struct nlmsghdr *)buf;
			while (NLMSG_OK(h, msglen)) {
				[...]
skip_it:
				h = NLMSG_NEXT(h, msglen);
			}
		}
		free(buf);
		[...]
	}

Thanks
Hangbin