[ovs-dev] ovsdb-server: Fix schema leak while reading db.

parse_txn() function doesn't always take ownership of the 'schema'
passed.  So, if the schema of the clustered db has same version as the
one that already in use, parse_txn() will not use it, resulting with a
memory leak:

 7,827 (56 direct, 7,771 indirect) bytes in 1 blocks are definitely lost
    at 0x483BB1A: calloc (vg_replace_malloc.c:762)
    by 0x44AD02: xcalloc (util.c:121)
    by 0x40E70E: ovsdb_schema_create (ovsdb.c:41)
    by 0x40EA6D: ovsdb_schema_from_json (ovsdb.c:217)
    by 0x415EDD: ovsdb_storage_read (storage.c:280)
    by 0x408968: read_db (ovsdb-server.c:607)
    by 0x40733D: main_loop (ovsdb-server.c:227)
    by 0x40733D: main (ovsdb-server.c:469)

While we could put ovsdb_schema_destroy() in a few places inside
'parse_txn()', from the users' point of view it seems better to have a
constant argument and just clone the 'schema' if needed.  The caller
will be responsible for destroying the 'schema' it owns.

CC: Ben Pfaff <blp@ovn.org>
Fixes: 1b1d2e6daa56 ("ovsdb: Introduce experimental support for clustered databases.")
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 ovsdb/ovsdb-server.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

On Fri, May 15, 2020 at 9:25 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> parse_txn() function doesn't always take ownership of the 'schema'
> passed.  So, if the schema of the clustered db has same version as the
> one that already in use, parse_txn() will not use it, resulting with a
> memory leak:
>
>  7,827 (56 direct, 7,771 indirect) bytes in 1 blocks are definitely lost
>     at 0x483BB1A: calloc (vg_replace_malloc.c:762)
>     by 0x44AD02: xcalloc (util.c:121)
>     by 0x40E70E: ovsdb_schema_create (ovsdb.c:41)
>     by 0x40EA6D: ovsdb_schema_from_json (ovsdb.c:217)
>     by 0x415EDD: ovsdb_storage_read (storage.c:280)
>     by 0x408968: read_db (ovsdb-server.c:607)
>     by 0x40733D: main_loop (ovsdb-server.c:227)
>     by 0x40733D: main (ovsdb-server.c:469)
>
> While we could put ovsdb_schema_destroy() in a few places inside
> 'parse_txn()', from the users' point of view it seems better to have a
> constant argument and just clone the 'schema' if needed.  The caller
> will be responsible for destroying the 'schema' it owns.
>
> CC: Ben Pfaff <blp@ovn.org>
> Fixes: 1b1d2e6daa56 ("ovsdb: Introduce experimental support for clustered
databases.")
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
>  ovsdb/ovsdb-server.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
> index d416f1b60..ef4e996df 100644
> --- a/ovsdb/ovsdb-server.c
> +++ b/ovsdb/ovsdb-server.c
> @@ -540,7 +540,7 @@ close_db(struct server_config *config, struct db *db,
char *comment)
>
>  static struct ovsdb_error * OVS_WARN_UNUSED_RESULT
>  parse_txn(struct server_config *config, struct db *db,
> -          struct ovsdb_schema *schema, const struct json *txn_json,
> +          const struct ovsdb_schema *schema, const struct json *txn_json,
>            const struct uuid *txnid)
>  {
>      if (schema && (!db->db->schema || strcmp(schema->version,
> @@ -565,7 +565,7 @@ parse_txn(struct server_config *config, struct db *db,
>               ? xasprintf("database %s schema changed", db->db->name)
>               : xasprintf("database %s connected to storage",
db->db->name)));
>
> -        ovsdb_replace(db->db, ovsdb_create(schema, NULL));
> +        ovsdb_replace(db->db, ovsdb_create(ovsdb_schema_clone(schema),
NULL));
>
>          /* Force update to schema in _Server database. */
>          db->row_uuid = UUID_ZERO;
> @@ -614,6 +614,7 @@ read_db(struct server_config *config, struct db *db)
>          } else {
>              error = parse_txn(config, db, schema, txn_json, &txnid);
>              json_destroy(txn_json);
> +            ovsdb_schema_destroy(schema);
>              if (error) {
>                  break;
>              }
> --
> 2.25.4
>

Acked-by: Han Zhou <hzhou@ovn.org>

On 5/27/20 1:52 AM, Han Zhou wrote:
> 
> 
> On Fri, May 15, 2020 at 9:25 AM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> wrote:
>>
>> parse_txn() function doesn't always take ownership of the 'schema'
>> passed.  So, if the schema of the clustered db has same version as the
>> one that already in use, parse_txn() will not use it, resulting with a
>> memory leak:
>>
>>  7,827 (56 direct, 7,771 indirect) bytes in 1 blocks are definitely lost
>>     at 0x483BB1A: calloc (vg_replace_malloc.c:762)
>>     by 0x44AD02: xcalloc (util.c:121)
>>     by 0x40E70E: ovsdb_schema_create (ovsdb.c:41)
>>     by 0x40EA6D: ovsdb_schema_from_json (ovsdb.c:217)
>>     by 0x415EDD: ovsdb_storage_read (storage.c:280)
>>     by 0x408968: read_db (ovsdb-server.c:607)
>>     by 0x40733D: main_loop (ovsdb-server.c:227)
>>     by 0x40733D: main (ovsdb-server.c:469)
>>
>> While we could put ovsdb_schema_destroy() in a few places inside
>> 'parse_txn()', from the users' point of view it seems better to have a
>> constant argument and just clone the 'schema' if needed.  The caller
>> will be responsible for destroying the 'schema' it owns.
>>
>> CC: Ben Pfaff <blp@ovn.org <mailto:blp@ovn.org>>
>> Fixes: 1b1d2e6daa56 ("ovsdb: Introduce experimental support for clustered databases.")
>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>>
>> ---
> 
> Acked-by: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>>

Thanks!  Applied to master and backported down to 2.9.

Best regards, Ilya Maximets.