Thanks, I pushed this.
On Fri, Sep 09, 2011 at 03:38:48PM -0700, Ethan Jackson wrote:
> 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
