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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 8 Sep 2017 14:32:38 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Phil Sutter <phil@....cc>, Hangbin Liu <liuhangbin@...il.com>,
        netdev@...r.kernel.org,
        Stephen Hemminger <stephen@...workplumber.org>
Subject: Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is
 not enough

On Fri, Sep 08, 2017 at 01:02:47PM +0200, Phil Sutter wrote:
> Hi Hangbin,
> 
> On Fri, Sep 08, 2017 at 06:14:56PM +0800, Hangbin Liu wrote:
> [...]
> > diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> > index be7ac86..37cfb5a 100644
> > --- a/lib/libnetlink.c
> > +++ b/lib/libnetlink.c
> > @@ -402,6 +402,59 @@ static void rtnl_dump_error(const struct rtnl_handle *rth,
> >  	}
> >  }
> >  
> > +static int rtnl_recvmsg(int fd, struct msghdr *msg, char **buf)
> > +{
> > +	struct iovec *iov;
> > +	int len = -1, buf_len = 32768;
> > +	char *buffer = *buf;
> 
> Isn't it possible to make 'buffer' static instead of the two 'buf'
> variables in rtnl_dump_filter_l() and __rtnl_talk()? Then we would have
> only a single buffer which is shared between both functions instead of
> two which are independently allocated.

Do we have to worry about reentrancy? Only arpd is multithreaded in
iproute2 but there might be also other users of libnetlink.

> > +
> > +	int flag = MSG_PEEK | MSG_TRUNC;
> > +
> > +	if (buffer == NULL)
> > +re_malloc:
> > +		buffer = malloc(buf_len);
> 
> I think using realloc() here is more appropriate since there is no need
> to free the buffer in beforehand and calling realloc(NULL, len) is
> equivalent to calling malloc(len). I think 'realloc' is also a better
> name for the goto label.
> 
> > +	if (buffer == NULL) {
> > +		fprintf(stderr, "malloc error: no enough buffer\n");
> 
> Minor typo here: s/no/not/
> 
> > +		return -1;
> 
> Return -ENOMEM?

Hm... the only caller of rtnl_dump_filter_l only checks if the return
value is negative. Even worse, at least some of the functions calling
__rtnl_talk() check errno (or use perror()) instead, even if it's not
always preserved. That's something for a wider cleanup, I would say.

> 
> > +	}
> > +
> > +	iov = msg->msg_iov;
> > +	iov->iov_base = buffer;
> > +	iov->iov_len = buf_len;
> > +
> > +re_recv:
> 
> Just call this 'recv'? (Not really important though.)
> 
> > +	len = recvmsg(fd, msg, flag);
> > +
> > +	if (len < 0) {
> > +		if (errno == EINTR || errno == EAGAIN)
> > +			return 0;
> 
> Instead of returning 0 (which makes callers retry), goto re_recv?
> 
> > +		fprintf(stderr, "netlink receive error %s (%d)\n",
> > +			strerror(errno), errno);
> > +		return len;
> > +	}
> > +
> > +	if (len == 0) {
> > +		fprintf(stderr, "EOF on netlink\n");
> > +		return -1;
> 
> Return -ENODATA here? (Initially I though about -EOF, but EOF is -1 so
> that would be incorrect).
> 
> > +	}
> > +
> > +	if (len > buf_len) {
> > +		free(buffer);
> 
> If you use realloc() above, this can be dropped.
> 
> > +		buf_len = len;
> 
> For this to work you have to make buf_len static too, otherwise you will
> unnecessarily reallocate the buffer. Oh, and that also requires the
> single buffer (as pointed out above) because you will otherwise use a
> common buf_len for both static buffers passed to this function.
> 
> > +		flag = 0;
> > +		goto re_malloc;
> > +	}
> > +
> > +	if (flag != 0) {
> > +		flag = 0;
> > +		goto re_recv;
> > +	}
> > +
> > +	*buf = buffer;
> > +	return len;
> > +}
> > +
> >  int rtnl_dump_filter_l(struct rtnl_handle *rth,
> >  		       const struct rtnl_dump_filter_arg *arg)
> >  {
> > @@ -413,31 +466,20 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
> >  		.msg_iov = &iov,
> >  		.msg_iovlen = 1,
> >  	};
> > -	char buf[32768];
> > +	static char *buf = NULL;
> 
> If you keep the static buffer in rtnl_recvmsg(), there is no need to
> assign NULL here.
> 
> >  	int dump_intr = 0;
> >  
> > -	iov.iov_base = buf;
> >  	while (1) {
> >  		int status;
> >  		const struct rtnl_dump_filter_arg *a;
> >  		int found_done = 0;
> >  		int msglen = 0;
> >  
> > -		iov.iov_len = sizeof(buf);
> > -		status = recvmsg(rth->fd, &msg, 0);
> > -
> > -		if (status < 0) {
> > -			if (errno == EINTR || errno == EAGAIN)
> > -				continue;
> > -			fprintf(stderr, "netlink receive error %s (%d)\n",
> > -				strerror(errno), errno);
> > -			return -1;
> > -		}
> > -
> > -		if (status == 0) {
> > -			fprintf(stderr, "EOF on netlink\n");
> > -			return -1;
> > -		}
> > +		status = rtnl_recvmsg(rth->fd, &msg, &buf);
> > +		if (status < 0)
> > +			return status;
> > +		else if (status == 0)
> > +			continue;
> 
> When retrying inside rtnl_recvmsg(), it won't return 0 anymore. I
> believe the whole 'while (1)' loop could go away then.

Doesn't this loop also handle the response divided into multiple
packets?

Michal Kubecek