Looks good,
Ethan
On Fri, Sep 9, 2011 at 10:21, Ben Pfaff <b...@nicira.com> wrote:
> Commit e408762f "netlink-socket: New function nl_lookup_genl_mcgroup()"
> modified do_lookup_genl_family() to return the Netlink attributes to the
> caller, but it still freed the Netlink message itself, which meant that
> the attributes pointed into freed memory. This commit fixes the problem.
>
> This commit is not a minimal fix. It refactors do_lookup_genl_family(),
> changing the return value from "negative errno value or positive genl
> family id" to the more common "zero or positive errno value".
>
> Found by valgrind.
> ---
> lib/netlink-socket.c | 71
> +++++++++++++++++++++++++++++---------------------
> 1 files changed, 41 insertions(+), 30 deletions(-)
>
> diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c
> index 394107e..2d2aa29 100644
> --- a/lib/netlink-socket.c
> +++ b/lib/netlink-socket.c
> @@ -727,45 +727,41 @@ genl_family_to_name(uint16_t id)
> }
>
> static int
> -do_lookup_genl_family(const char *name, struct nlattr **attrs)
> +do_lookup_genl_family(const char *name, struct nlattr **attrs,
> + struct ofpbuf **replyp)
> {
> struct nl_sock *sock;
> struct ofpbuf request, *reply;
> - int retval;
> + int error;
>
> - retval = nl_sock_create(NETLINK_GENERIC, &sock);
> - if (retval) {
> - return -retval;
> + *replyp = NULL;
> + error = nl_sock_create(NETLINK_GENERIC, &sock);
> + if (error) {
> + return error;
> }
>
> ofpbuf_init(&request, 0);
> nl_msg_put_genlmsghdr(&request, 0, GENL_ID_CTRL, NLM_F_REQUEST,
> CTRL_CMD_GETFAMILY, 1);
> nl_msg_put_string(&request, CTRL_ATTR_FAMILY_NAME, name);
> - retval = nl_sock_transact(sock, &request, &reply);
> + error = nl_sock_transact(sock, &request, &reply);
> ofpbuf_uninit(&request);
> - if (retval) {
> + if (error) {
> nl_sock_destroy(sock);
> - return -retval;
> + return error;
> }
>
> if (!nl_policy_parse(reply, NLMSG_HDRLEN + GENL_HDRLEN,
> - family_policy, attrs, ARRAY_SIZE(family_policy))) {
> + family_policy, attrs, ARRAY_SIZE(family_policy))
> + || nl_attr_get_u16(attrs[CTRL_ATTR_FAMILY_ID]) == 0) {
> nl_sock_destroy(sock);
> ofpbuf_delete(reply);
> - return -EPROTO;
> + return EPROTO;
> }
>
> - retval = nl_attr_get_u16(attrs[CTRL_ATTR_FAMILY_ID]);
> - if (retval == 0) {
> - retval = -EPROTO;
> - } else {
> - define_genl_family(retval, name);
> - }
> nl_sock_destroy(sock);
> - ofpbuf_delete(reply);
> -
> - return retval;
> + *replyp = reply;
> + return 0;
> }
>
> /* Finds the multicast group called 'group_name' in genl family
> 'family_name'.
> @@ -777,15 +773,15 @@ nl_lookup_genl_mcgroup(const char *family_name, const
> char *group_name,
> {
> struct nlattr *family_attrs[ARRAY_SIZE(family_policy)];
> struct ofpbuf all_mcs;
> + struct ofpbuf *reply;
> struct nlattr *mc;
> unsigned int left;
> - int retval;
> + int error;
>
> *multicast_group = 0;
> - retval = do_lookup_genl_family(family_name, family_attrs);
> - if (retval <= 0) {
> - assert(retval);
> - return -retval;
> + error = do_lookup_genl_family(family_name, family_attrs, &reply);
> + if (error) {
> + return error;
> }
>
> nl_attr_get_nested(family_attrs[CTRL_ATTR_MCAST_GROUPS], &all_mcs);
> @@ -799,18 +795,23 @@ nl_lookup_genl_mcgroup(const char *family_name, const
> char *group_name,
> const char *mc_name;
>
> if (!nl_parse_nested(mc, mc_policy, mc_attrs, ARRAY_SIZE(mc_policy)))
> {
> - return EPROTO;
> + error = EPROTO;
> + goto exit;
> }
>
> mc_name = nl_attr_get_string(mc_attrs[CTRL_ATTR_MCAST_GRP_NAME]);
> if (!strcmp(group_name, mc_name)) {
> *multicast_group =
> nl_attr_get_u32(mc_attrs[CTRL_ATTR_MCAST_GRP_ID]);
> - return 0;
> + error = 0;
> + goto exit;
> }
> }
> + error = EPROTO;
>
> - return EPROTO;
> +exit:
> + ofpbuf_delete(reply);
> + return error;
> }
>
> /* If '*number' is 0, translates the given Generic Netlink family 'name' to a
> @@ -820,10 +821,20 @@ nl_lookup_genl_mcgroup(const char *family_name, const
> char *group_name,
> int
> nl_lookup_genl_family(const char *name, int *number)
> {
> - struct nlattr *attrs[ARRAY_SIZE(family_policy)];
> -
> if (*number == 0) {
> - *number = do_lookup_genl_family(name, attrs);
> + struct nlattr *attrs[ARRAY_SIZE(family_policy)];
> + struct ofpbuf *reply;
> + int error;
> +
> + error = do_lookup_genl_family(name, attrs, &reply);
> + if (!error) {
> + *number = nl_attr_get_u16(attrs[CTRL_ATTR_FAMILY_ID]);
> + define_genl_family(*number, name);
> + } else {
> + *number = -error;
> + }
> + ofpbuf_delete(reply);
> +
> assert(*number != 0);
> }
> return *number > 0 ? 0 : -*number;
> --
> 1.7.4.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
