| Message ID | 20221208011122.2343363-13-jesse.brandeburg@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Michal Kubecek |
| Headers | show |
| Series | ethtool: clean up and fix | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, Dec 07, 2022 at 05:11:21PM -0800, Jesse Brandeburg wrote: > cppcheck finds: > netlink/msgbuff.c:63:2: error: Memory leak: nbuff [memleak] > return 0; > ^ This reported error is misleading, the commit message should make it clear that the leak it addresses is not of nbuff but of the original msgbuff->buff (on failure). > This is a pretty common problem with realloc() and just requires handling > the return code correctly which makes us refactor to reuse the structure > free/reinit code that already exists in msgbuf_done(). > > This fixes the code flow by doing the right thing if realloc() succeeds and > if it fails then being sure to free the original memory and replicate the > steps the original code took. > > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > --- > netlink/msgbuff.c | 39 ++++++++++++++++++--------------------- > 1 file changed, 18 insertions(+), 21 deletions(-) > > diff --git a/netlink/msgbuff.c b/netlink/msgbuff.c > index 216f5b946236..a599cab06014 100644 > --- a/netlink/msgbuff.c > +++ b/netlink/msgbuff.c [...] > @@ -43,19 +57,16 @@ int msgbuff_realloc(struct nl_msg_buff *msgbuff, unsigned int new_size) > if (new_size > MAX_MSG_SIZE) > return -EMSGSIZE; > nbuff = realloc(msgbuff->buff, new_size); > - if (!nbuff) { > - msgbuff->buff = NULL; > - msgbuff->size = 0; > - msgbuff->left = 0; > - return -ENOMEM; > - } > - if (nbuff != msgbuff->buff) { > + if (nbuff) { > if (new_size > old_size) > memset(nbuff + old_size, '\0', new_size - old_size); > msgbuff->nlhdr = (struct nlmsghdr *)(nbuff + nlhdr_off); > msgbuff->genlhdr = (struct genlmsghdr *)(nbuff + genlhdr_off); > msgbuff->payload = nbuff + payload_off; > msgbuff->buff = nbuff; > + } else { > + msgbuff_done(msgbuff); > + return -ENOMEM; > } > msgbuff->size = new_size; > msgbuff->left += (new_size - old_size); If nbuff is null (reallocation failed), we bail out anyway so there is no point putting part of the followin code inside an if and leaving the rest outside. Also, while it makes sense to zero the new part of the buffer even if the buffer is not moved, there is no reason to reinitialize all the pointers. How about this: if (!nbuff) { msgbuff_done(msgbuff); return -ENOMEM; } if (new_size > old_size) memset(nbuff + old_size, '\0', new_size - old_size); if (nbuff != msgbuff->buff) { msgbuff->nlhdr = (struct nlmsghdr *)(nbuff + nlhdr_off); msgbuff->genlhdr = (struct genlmsghdr *)(nbuff + genlhdr_off); msgbuff->payload = nbuff + payload_off; msgbuff->buff = nbuff; } msgbuff->size = new_size; msgbuff->left += (new_size - old_size); Or we could forget about the "if (nbuff != msgbuff->buff)" test and assign the four pointers to values they already have in case the block stays in place. The code would look a bit tidier and the difference would be negligible. And looking at the code again, the "new_size > old_size" check can be omitted too as we already have if (new_size <= old_size) return 0; before the realloc() call. This is a relic from a development version where realloc() was also used to shrink the buffer if new requested size was smaller than current. Michal
diff --git a/netlink/msgbuff.c b/netlink/msgbuff.c index 216f5b946236..a599cab06014 100644 --- a/netlink/msgbuff.c +++ b/netlink/msgbuff.c @@ -15,6 +15,20 @@ #define MAX_MSG_SIZE (4 << 20) /* 4 MB */ +/** + * msg_done() - destroy a message buffer + * @msgbuff: message buffer + * + * Free the buffer and reset size and remaining size. + */ +void msgbuff_done(struct nl_msg_buff *msgbuff) +{ + free(msgbuff->buff); + msgbuff->buff = NULL; + msgbuff->size = 0; + msgbuff->left = 0; +} + /** * msgbuff_realloc() - reallocate buffer if needed * @msgbuff: message buffer @@ -43,19 +57,16 @@ int msgbuff_realloc(struct nl_msg_buff *msgbuff, unsigned int new_size) if (new_size > MAX_MSG_SIZE) return -EMSGSIZE; nbuff = realloc(msgbuff->buff, new_size); - if (!nbuff) { - msgbuff->buff = NULL; - msgbuff->size = 0; - msgbuff->left = 0; - return -ENOMEM; - } - if (nbuff != msgbuff->buff) { + if (nbuff) { if (new_size > old_size) memset(nbuff + old_size, '\0', new_size - old_size); msgbuff->nlhdr = (struct nlmsghdr *)(nbuff + nlhdr_off); msgbuff->genlhdr = (struct genlmsghdr *)(nbuff + genlhdr_off); msgbuff->payload = nbuff + payload_off; msgbuff->buff = nbuff; + } else { + msgbuff_done(msgbuff); + return -ENOMEM; } msgbuff->size = new_size; msgbuff->left += (new_size - old_size); @@ -240,17 +251,3 @@ void msgbuff_init(struct nl_msg_buff *msgbuff) { memset(msgbuff, '\0', sizeof(*msgbuff)); } - -/** - * msg_done() - destroy a message buffer - * @msgbuff: message buffer - * - * Free the buffer and reset size and remaining size. - */ -void msgbuff_done(struct nl_msg_buff *msgbuff) -{ - free(msgbuff->buff); - msgbuff->buff = NULL; - msgbuff->size = 0; - msgbuff->left = 0; -}
cppcheck finds: netlink/msgbuff.c:63:2: error: Memory leak: nbuff [memleak] return 0; ^ This is a pretty common problem with realloc() and just requires handling the return code correctly which makes us refactor to reuse the structure free/reinit code that already exists in msgbuf_done(). This fixes the code flow by doing the right thing if realloc() succeeds and if it fails then being sure to free the original memory and replicate the steps the original code took. Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> --- netlink/msgbuff.c | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-)