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

[Damijan Skvarc]

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
> >> > >
>