[ovs-dev] [PATCH 1/3] dpif: Fix NULL dereference fetching non-UFID flow.

[Ben Pfaff]

On Thu, Dec 11, 2014 at 03:40:47PM -0800, Joe Stringer wrote:
> On 11 December 2014 at 15:09, Ben Pfaff <blp at nicira.com> wrote:
> > On Thu, Dec 11, 2014 at 02:48:46PM -0800, Joe Stringer wrote:
> >> On 11 December 2014 at 14:05, Ben Pfaff <blp at nicira.com> wrote:
> >> > On Wed, Dec 03, 2014 at 02:24:46PM -0800, Joe Stringer wrote:
> >> >> The UFID parameter to dpif_flow_get() is optional, but the current
> >> >> implementation dereferences it to initialize part of the output flow.
> >> >> This field is filled in by the dpif implementation, so don't initialize
> >> >> it here.
> >> >>
> >> >> This could cause a NULL dereference if a dpif_flow_get() caller doesn't
> >> >> provide a UFID. Currently there are no such callers, but the next patch
> >> >> will introduce one.
> >> >>
> >> >> Signed-off-by: Joe Stringer <joestringer at nicira.com>
> >> >
> >> > Acked-by: Ben Pfaff <blp at nicira.com>
> >> >
> >> > The title of the patch makes this sound like a bug fix, but the
> >> > explanation later on sounds like it would only be a real bug if the
> >> > callers were changed.  Is there a bug that can currently be triggered?
> >>
> >> No, it cannot be triggered currently. Perhaps this could be rolled
> >> into the next patch, or renamed.
> >
> > I'd just adjust the commit message to make it even clearer.  Maybe
> > something like "This does not fix any existing bug because every caller
> > currently passes in a UFID."
> 
> OK, thanks for the review. I will push this series soon with the log
> updated as below:
> 
>     dpif: Don't initialize output UFID in dpif_flow_get().
> 
>     The UFID parameter to dpif_flow_get() is optional, but the current
>     implementation dereferences it to initialize part of the output flow.
>     This field is filled in by the dpif implementation, so don't initialize
>     it here.
> 
>     This does not fix any existing bug because every caller currently passes
>     in a UFID. The next patch will introduce the first call to
>     dpif_flow_get() that doesn't provide a UFID, which would break without
>     this change.

Thanks!