Hi Roger, Thanks for looking into it and for your reply.I don't see a compulsory reason to change the code. In context of JDK-8217375 I happened to touch the issue and I agree to Max to solve it differently in that particular case now by creating a real deep copy based on reading the manifest bytes input stream again.h ttps://mail.openjdk.java.net/pipermail/security-dev/2019- January/019200.html Even though I don't have a strong reason to change Manifest code any more, in my opinion the current actual behavior is quite weird, possibly too strange even to properly document without deprecating it at the same time. Chances are that other people struggled over it just like I did and will so again always getting the impression of a bug whether documented or not. The current copy constructor already now tries to deeply copy the original but clones only two out of three collections. Hence, I'd guess the current code was developed with the intention to create a deep copy back when it was developed. Maintaining perfect backward compatibility would mean to also add a new deepClone or similar method along with another constructor wouldn't it? The are probably other uses but the only use of individual sections in manifests that I'm currently aware of is jar signing and there it has not been found so far as I'm aware of. While there certainly are risks to clients there might be chances as well to fix issues in client code. These are just some considerations in favor of changing the code but by no means meant as a conclusion. Philipp
On Tue, 2019-01-22 at 15:00 -0500, Roger Riggs wrote: > Hi Phillip, > > Did you discover need for a deeper copy? If so, can you elaborate? > It may be that adding a new constructor would be cleaner. > > I'd suggest clarifying the documentation of existing methods as > opposed > to changing the spec and implementation and the possible risks > to clients. > > Thanks, Roger > > On 01/21/2019 05:50 PM, Lance Andersen wrote: > > Hi Philipp, > > > > This is going to take some further discussion/input as this has > > been documented for sometime as a shallow copy. > > > > If there is a consensus to move this forward, it will also require > > a CSR. I can help with that and sponsoring the updates, if the > > decision is to go ahead with the proposed change > > > > Best > > Lance > > > On Jan 19, 2019, at 2:34 PM, Philipp Kunz <philipp.kunz@paratix.c > > > h> wrote: > > > > > > Hi again, > > > Just after having sent the previous mail I found Manifest::clone > > > explicitly says it creates a shallow copy which is true only to a > > > certain extent. It deeply copies main attributes as well as > > > individual > > > sections map already now and only shallowly copies individual > > > sections > > > attributes maps. > > > I don't know about the background of it or why clone should only > > > do a > > > shallow copy but if clone should really create a shallow copy, it > > > should probably create an even more shallow copy. In any case I > > > figure > > > some potential for clarification at least. > > > Probably mostly because I already began a patch in the previous > > > message, I continued and attached another patch for a deep copy > > > approach. There might still be some reason not to deeply copy > > > manifests > > > which I don't intend to forestall. > > > Philipp > > > > > > On Sat, 2019-01-19 at 19:32 +0100, Philipp Kunz wrote: > > > > Hi, > > > > > > > > Creating a new manifest as a copy from an existing one using > > > > the copy > > > > constructor assigns the new manifest individual sections > > > > entries map > > > > the same values which are references to attributes as the > > > > original > > > > rather than copying them as well deeply resulting in two > > > > manifests > > > > pointing to the same attributes instances modifications to > > > > which > > > > always affect both manifests. See test and proposed fix in the > > > > attached patch. > > > > > > > > Regards, > > > > Philipp > > > > > > <manifestcopyconstructorandclone.patch> > > > > <http://oracle.com/us/design/oracle-email-sig-198324.gif> > > <http://oracle.com/us/design/oracle-email-sig-198324.gif> > > <http://oracle.com/us/design/oracle-email-sig-198324.gif> > > <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance > > Andersen| Principal Member of Technical Staff | +1.781.442.2037 > > Oracle Java Engineering > > 1 Network Drive > > Burlington, MA 01803 > > lance.ander...@oracle.com <mailto:lance.ander...@oracle.com> > > > > > >
