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.skv...@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 <dce...@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.skv...@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, <b...@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, <b...@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 >> >> > > d...@openvswitch.org >> >> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
