diff mbox series

[ovs-dev,02/11] ovn-controller: Fix possible null pointer in memcmp.

Message ID 1509211918-14829-3-git-send-email-u9012063@gmail.com
State Changes Requested
Headers show
Series Fix clang static analysis null pointer bugs. | expand

Commit Message

William Tu Oct. 28, 2017, 5:31 p.m. UTC
Clang reports possible null pointer in_dhcp_opt passing to memcmp.
This might due to dp_packet_get_udp_payload retuning null.  Fix it
by adding ovs_assert.

Signed-off-by: William Tu <u9012063@gmail.com>
---
 ovn/controller/pinctrl.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Mark Michelson Oct. 30, 2017, 2:26 p.m. UTC | #1
This looks fine to me. Usually I don't like assertions based on contents of
incoming packets. However, in this case, ovn-northd has set up the logical
flow so that only UDP packets can reach here.

On Sat, Oct 28, 2017 at 12:34 PM William Tu <u9012063@gmail.com> wrote:

> Clang reports possible null pointer in_dhcp_opt passing to memcmp.
> This might due to dp_packet_get_udp_payload retuning null.  Fix it
> by adding ovs_assert.
>
> Signed-off-by: William Tu <u9012063@gmail.com>
>
Acked-by: Mark Michelson<mmichels@redhat.com>

> ---
>  ovn/controller/pinctrl.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 469a35586b8a..3fdd01182a52 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -270,6 +270,7 @@ pinctrl_handle_put_dhcp_opts(
>          sizeof (struct dhcp_header);
>
>      ovs_be32 magic_cookie = htonl(DHCP_MAGIC_COOKIE);
> +    ovs_assert(in_dhcp_opt);
>      if (memcmp(in_dhcp_opt, &magic_cookie, sizeof(ovs_be32))) {
>          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>          VLOG_WARN_RL(&rl, "DHCP magic cookie not present in the DHCP
> packet");
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ben Pfaff Oct. 30, 2017, 7:15 p.m. UTC | #2
On Sat, Oct 28, 2017 at 10:31:49AM -0700, William Tu wrote:
> Clang reports possible null pointer in_dhcp_opt passing to memcmp.
> This might due to dp_packet_get_udp_payload retuning null.  Fix it
> by adding ovs_assert.
> 
> Signed-off-by: William Tu <u9012063@gmail.com>
> ---
>  ovn/controller/pinctrl.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 469a35586b8a..3fdd01182a52 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -270,6 +270,7 @@ pinctrl_handle_put_dhcp_opts(
>          sizeof (struct dhcp_header);
>  
>      ovs_be32 magic_cookie = htonl(DHCP_MAGIC_COOKIE);
> +    ovs_assert(in_dhcp_opt);

I don't see how this makes sense.  in_dhcp_opt is a pointer plus a
nonzero constant.  That's only null if it wraps around.  I don't want to
"fix" this one.
diff mbox series

Patch

diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 469a35586b8a..3fdd01182a52 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -270,6 +270,7 @@  pinctrl_handle_put_dhcp_opts(
         sizeof (struct dhcp_header);
 
     ovs_be32 magic_cookie = htonl(DHCP_MAGIC_COOKIE);
+    ovs_assert(in_dhcp_opt);
     if (memcmp(in_dhcp_opt, &magic_cookie, sizeof(ovs_be32))) {
         static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
         VLOG_WARN_RL(&rl, "DHCP magic cookie not present in the DHCP packet");