diff mbox series

[ovs-dev,PATCHv2] ovsdb-idl: Fix NULL deref reported by Coverity.

Message ID 1589550415-99935-1-git-send-email-u9012063@gmail.com
State New
Headers show
Series [ovs-dev,PATCHv2] ovsdb-idl: Fix NULL deref reported by Coverity. | expand

Commit Message

William Tu May 15, 2020, 1:46 p.m. UTC
When 'datum.values' or 'datum.keys' is NULL, some code path calling
into ovsdb_idl_txn_write__ triggers NULL deref.

An example:
ovsrec_open_vswitch_set_cur_cfg(const struct ovsrec_open_vswitch
{
    struct ovsdb_datum datum;
    union ovsdb_atom key;

    datum.n = 1;
    datum.keys = &key;

    key.integer = cur_cfg;
//  1. assign_zero: Assigning: datum.values = NULL.
    datum.values = NULL;
//  CID 1421356 (#1 of 1): Explicit null dereferenced (FORWARD_NULL)
//  2. var_deref_model: Passing &datum to ovsdb_idl_txn_write_clone,\
//     which dereferences null datum.values.
    ovsdb_idl_txn_write_clone(&row->header_, &ovsrec_open_vswitch_col
}

And with the following calls:
ovsdb_idl_txn_write_clone
  ovsdb_idl_txn_write__
    6. deref_parm_in_call: Function ovsdb_datum_destroy dereferences
       datum->values
    ovsdb_datum_destroy

Signed-off-by: William Tu <u9012063@gmail.com>
---
v2:
   - I applied previous patch e398275024e815b52with yifeng's comments,
     but accidently remove this chunk.  With this fix, the Coverity
     shows around 133 defects. (now it's around 300)
---
 lib/ovsdb-idl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Yifeng Sun May 18, 2020, 9:14 p.m. UTC | #1
Thanks William.

Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>


On Fri, May 15, 2020 at 6:47 AM William Tu <u9012063@gmail.com> wrote:

> When 'datum.values' or 'datum.keys' is NULL, some code path calling
> into ovsdb_idl_txn_write__ triggers NULL deref.
>
> An example:
> ovsrec_open_vswitch_set_cur_cfg(const struct ovsrec_open_vswitch
> {
>     struct ovsdb_datum datum;
>     union ovsdb_atom key;
>
>     datum.n = 1;
>     datum.keys = &key;
>
>     key.integer = cur_cfg;
> //  1. assign_zero: Assigning: datum.values = NULL.
>     datum.values = NULL;
> //  CID 1421356 (#1 of 1): Explicit null dereferenced (FORWARD_NULL)
> //  2. var_deref_model: Passing &datum to ovsdb_idl_txn_write_clone,\
> //     which dereferences null datum.values.
>     ovsdb_idl_txn_write_clone(&row->header_, &ovsrec_open_vswitch_col
> }
>
> And with the following calls:
> ovsdb_idl_txn_write_clone
>   ovsdb_idl_txn_write__
>     6. deref_parm_in_call: Function ovsdb_datum_destroy dereferences
>        datum->values
>     ovsdb_datum_destroy
>
> Signed-off-by: William Tu <u9012063@gmail.com>
> ---
> v2:
>    - I applied previous patch e398275024e815b52with yifeng's comments,
>      but accidently remove this chunk.  With this fix, the Coverity
>      shows around 133 defects. (now it's around 300)
> ---
>  lib/ovsdb-idl.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index 1535ad7b5197..6614ea1617ef 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -4449,7 +4449,8 @@ ovsdb_idl_txn_write__(const struct ovsdb_idl_row
> *row_,
>       * transaction only does writes of existing values, without making
> any real
>       * changes, we will drop the whole transaction later in
>       * ovsdb_idl_txn_commit().) */
> -    if (write_only && ovsdb_datum_equals(ovsdb_idl_read(row, column),
> +    if (datum->keys && datum->values &&
> +        write_only && ovsdb_datum_equals(ovsdb_idl_read(row, column),
>                                           datum, &column->type)) {
>          goto discard_datum;
>      }
> --
> 2.7.4
>
>
William Tu May 20, 2020, 3:53 p.m. UTC | #2
On Mon, May 18, 2020 at 02:14:36PM -0700, Yifeng Sun wrote:
> Thanks William.
> 
> Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>
> 
Thanks, applied to master.
William
diff mbox series

Patch

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 1535ad7b5197..6614ea1617ef 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -4449,7 +4449,8 @@  ovsdb_idl_txn_write__(const struct ovsdb_idl_row *row_,
      * transaction only does writes of existing values, without making any real
      * changes, we will drop the whole transaction later in
      * ovsdb_idl_txn_commit().) */
-    if (write_only && ovsdb_datum_equals(ovsdb_idl_read(row, column),
+    if (datum->keys && datum->values &&
+        write_only && ovsdb_datum_equals(ovsdb_idl_read(row, column),
                                          datum, &column->type)) {
         goto discard_datum;
     }