search

encryptogroup / OTExtension

C++ OT extension implementation
GNU Lesser General Public License v3.0
125 stars 35 forks source link

Fix memory errors #9

Closed schoppmp closed 7 years ago

schoppmp commented 7 years ago

This closes several memory leaks, mainly by implementing destructors for the OTExt subclasses. Some of the cleanup code could be done in the base class (OTExt) instead of the subclasses (OTExtSnd, OTExtRec), but that would need an additional attribute. I think that makes it basically a design choice, I'd be happy with either solution. See also the comment in the OTExtRec destructor.

dd23 commented 7 years ago

Thanks a lot for the pull request! We're very grateful for your help with this ongoing issue. We'll review your changes and will probably merge it within the next few days.

dd23 commented 7 years ago

Just a quick remark: 3262e78 does not compile, since you haven't included the modified util/channel.h, that you updated in your branch of ABY. This file currently needs to be updated in both OTExtension and ABY. Maybe we should move the entire utils to a separate submodule…

lenerd commented 7 years ago

This file currently needs to be updated in both OTExtension and ABY. Maybe we should move the entire utils to a separate submodule…

I would appreciate that.

When I wanted to compile ABY and OTExtension as separate libraries the doubled symbols made things complicated. So I tried to omit the utils from one project, but there were subtle differences in the utils source code. For example some class had an additional attribute in one repository but not the other. Including the header of ABY utils and linking against the OTExtension utils or vice versa resulted in strange errors.

dd23 commented 7 years ago

Ok, good point. Then I'd take care of that very soon. One question: should I do that based on the current public branches, or should I already include the memory fixes of @schoppmp? I'd only merge his changes to files in util if he tells me that they are somewhat stable and ready to be merged. Otherwise we would of course merge his changes later on.

schoppmp commented 7 years ago

Thanks for pointing that out. I just pushed a commit that adds the missing file. Now, everything should build properly. I think moving utils to a new submodule is a very good idea. I also had problems when trying to combine different projects that all brought their own version of it. My changes to utils aren't many, and I think they are stable. You can have a look at the diffs here to make sure: https://github.com/encryptogroup/ABY/pull/27/files#diff-8282cf584dd3e6f6ddc3e155cab81dcb

lenerd commented 7 years ago

@dd23 I have no objection against including the fixes. The current version is still available via git.