[ovs-dev] [PATCH] ovsdb-idl: Mark row "parsed" in ovsdb_idl_txn_write__

Dumitru Ceara dceara at redhat.com
Mon Jul 29 08:52:18 UTC 2019


Hi Ben,

If we keep this fix as is can we please have it backported all the way
to and including branch-2.10? Right now it's there in master and
branch 2.12.

ovn-nbctl daemon mode is available since branch-2.10 and might be
affected by this leak as it creates new entries in the database.

Thanks,
Dumitru

On Mon, Jul 22, 2019 at 9:58 AM Damijan Skvarc <damjan.skvarc at gmail.com> wrote:
>
> Hi Dumitru,
>
> The problem is that ovsdb_idl entity is part of library and can be used by different application, where each application
> instantiates its own parse/unparse callback functions. Library itself does not know how these parse/unparse functions are
> implemented thus it is not reliable to call unparse() function without calling  apparent parse() function first. This case
> happens in case the application modifies one column, while common parse flag insinuates unparse()
> function of some another column is called.  The most problematic case are parse/unparse pairs where parse()
> function allocates memory and its unparse() counterpart deallocates it. Common parse flag would in this case cause some
> memory would be freed despite it has not been allocated or even worse, some memory could be deallocated multiple times.
> I strongly believe this would lead soon or later to the application crash.
>
> However, since I am just a self-invited visitor on this project (just to gain/discover some practices on github platform) you should
> not rely on my opinion. Ben looks to be a moderator/architect of this project and he should advice how to precede with this issue.
>
> regards,
> Damijan
>
>
> On Fri, Jul 19, 2019 at 10:12 AM Dumitru Ceara <dceara at redhat.com> wrote:
>>
>> Hi Damijan, Ben,
>>
>> Damijan, sorry, I didn't realize you had already reported and fixed
>> the problem in your pending pull request. I wouldn't have sent my
>> patch otherwise.
>>
>> Regarding a single parsed field instead of parsed bits per column I
>> had the impression that it's ok if unparsing is done for all columns.
>> Right now, whenever we set a column we call ovsdb_idl_txn_write__()
>> which unconditionally executes "(column->unparse)(row)" even if there
>> was no old value set before parsing the new datum. As we zero out rows
>> when we allocate them I assumed that this unparsing is safe.
>>
>> What do you guys think?
>>
>> Thanks,
>> Dumitru
>>
>> On Fri, Jul 19, 2019 at 4:50 AM Damijan Skvarc <damjan.skvarc at gmail.com> wrote:
>> >
>> > https://mail.openvswitch.org/pipermail/ovs-dev/2019-July/360285.htmlhttps://mail.openvswitch.org/pipermail/ovs-dev/2019-July/360285.html
>> >
>> > On Thu, 18 Jul 2019, 20:12 Ben Pfaff, <blp at ovn.org> wrote:
>> >>
>> >> I guess I missed it.  Will you please point it out to me, for example in
>> >> the list archive?
>> >>
>> >> On Thu, Jul 18, 2019 at 06:58:34PM +0200, Damijan Skvarc wrote:
>> >> > Hmm, the problem of "parsed" flag is that it identifies "all" columns of
>> >> > certain row have been parsed, however there are CLI tools which modify only
>> >> > individual colums by calling ovsdb_idl_txn_write_() function. In this case
>> >> > and in case parsed flag would be set in ovsdb_idl_txn_write() function then
>> >> > unparsing procedure would be called also for columns which were not parsed.
>> >> > The problem could be overcome by having individual flag for each column.
>> >> > This has been addresed in pending pull request. Apparent mail has been sent
>> >> > to dev list, but obviosly has been somehow overlooked.
>> >> > br, damijan
>> >> >
>> >> > On Thu, 18 Jul 2019, 17:52 Ben Pfaff, <blp at ovn.org> wrote:
>> >> >
>> >> > > On Wed, Jul 17, 2019 at 09:05:04PM +0200, Dumitru Ceara wrote:
>> >> > > > Once a column is set in a row using ovsdb_idl_txn_write__ we also mark
>> >> > > > the row "parsed". Otherwise the memory allocated by
>> >> > > > sbrec_<table>_parse_<col>() will never be freed. After marking the row
>> >> > > > "parsed", the ovsdb_idl_txn_disassemble function will properly free the
>> >> > > > newly allocated memory for the column (ovsdb_idl_row_unparse).
>> >> > >
>> >> > > Wow, good catch.  I bet that's been there forever.
>> >> > >
>> >> > > The OVSDB IDL code is too complicated.  It's too hard to spot the
>> >> > > issues.  I wish I saw a way to make it simpler.
>> >> > > _______________________________________________
>> >> > > dev mailing list
>> >> > > dev at openvswitch.org
>> >> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> >> > >


More information about the dev mailing list