On Tue, 8 Mar 2022 04:09:23 GMT, Sergey Bylokhov <s...@openjdk.org> wrote:
> The current bug looks similar to this one > https://bugs.openjdk.java.net/browse/JDK-8272865 I tried to fix it the same > way but I was not sure if it is safe to ignore the ReadWriteProfileTest.java > or not, since it has five related bugids. I missed that bug. There's overlap but this bug is mainly about an unusable profile being returned. The "nicety" that getData() returns what setData() set is much less important. However I have been pondering that test. If by the 5 related bug IDs you mean those in the @bug tag * @bug 6476665 6523403 6733501 7042594 7043064 only the last of them says anything clearly related where the eval from 2013 says -- The only exception is ReadWriteProfileTest. This test relies on an assumption, that we read a data for a tag in the exactly same form as it was written. However, this assumption is not correct in case of lcms, because this library may perform some sort of modifications on the tag data. For example, plain text tags can be promoted to multilinguage tags, color points can be normalized and etc. To get this test working, we ether have to upgrade the tag comparison routines in the test, or force "as is" tag injection in lcms. The second option seems to be a bit dangerous to me because it potentially open doors for an injection of malformed tags into a profile. --- Actually it is not clear to me that modifications to that effect were made to the test by that bug fix. I only see it skipping Kodak tags : http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/a07c907a82b5 But it means even back then there was acknowledgement that getData() may not return what setData() set even if the data were accepted. And plain (ascii) text tags being promoted to multi-lingual tags is an example of what I saw. I've not dug into locating all versions of the ICC spec but an old copy makes no mention of the multi-lingual support so I can imagine that LittleCMS is "bringing up to date" a tag that was encoded into a profile before that support was present. Source comments in LittleCMS on cmsReadRawTag seemed to suggest it would always cook the tag so as to make sure it returned consistent results. But I checked with the LCMS maintainer and he said that was out-dated and updated the source comments. So it is definite that we can't rely on these being consistent. So I have been wondering if the test is even valid and worth saving. The spec doesn't say anything one way or another https://docs.oracle.com/en/java/javase/17/docs/api/java.desktop/java/awt/color/ICC_Profile.html#getData() so I am sure a case could be made either way as to what it intended. One way to "save" the test is that after doing the validation which calls cmsReadTag() and get an "OK" we discard this profile and create ANOTHER copy which we return to the app. Since nothing will have cooked that new profile data it will get back what it set. Unless something else cooks it or LittleCMS behaviour changes. I'm up for doing that here and then the test will pass again without changes. > The current bug looks similar to this one > https://bugs.openjdk.java.net/browse/JDK-8272865 I tried to fix it the same > way but I was not sure if it is safe to ignore the ReadWriteProfileTest.java > or not, since it has five related bugids. I missed that bug. There's overlap but this bug is mainly about an unusable profile being returned. The "nicety" that getData() returns what setData() set is much less important. However I have been pondering that test. If by the 5 related bug IDs you mean those in the @bug tag * @bug 6476665 6523403 6733501 7042594 7043064 only the last of them says anything clearly related where the eval from 2013 says -- The only exception is ReadWriteProfileTest. This test relies on an assumption, that we read a data for a tag in the exactly same form as it was written. However, this assumption is not correct in case of lcms, because this library may perform some sort of modifications on the tag data. For example, plain text tags can be promoted to multilinguage tags, color points can be normalized and etc. To get this test working, we ether have to upgrade the tag comparison routines in the test, or force "as is" tag injection in lcms. The second option seems to be a bit dangerous to me because it potentially open doors for an injection of malformed tags into a profile. --- Actually it is not clear to me that modifications to that effect were made to the test by that bug fix. I only see it skipping Kodak tags : http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/a07c907a82b5 But it means even back then there was acknowledgement that getData() may not return what setData() set even if the data were accepted. And plain (ascii) text tags being promoted to multi-lingual tags is an example of what I saw. I've not dug into locating all versions of the ICC spec but an old copy makes no mention of the multi-lingual support so I can imagine that LittleCMS is "bringing up to date" a tag that was encoded into a profile before that support was present. Source comments in LittleCMS on cmsReadRawTag seemed to suggest it would always cook the tag so as to make sure it returned consistent results. But I checked with the LCMS maintainer and he said that was out-dated and updated the source comments. So it is definite that we can't rely on these being consistent. So I have been wondering if the test is even valid and worth saving. The spec doesn't say anything one way or another https://docs.oracle.com/en/java/javase/17/docs/api/java.desktop/java/awt/color/ICC_Profile.html#getData() so I am sure a case could be made either way as to what it intended. One way to "save" the test is that after doing the validation which calls cmsReadTag() and get an "OK" we discard this profile and create ANOTHER copy which we return to the app. Since nothing will have cooked that new profile data it will get back what it set. Unless something else cooks it or LittleCMS behaviour changes. I'm up for doing that here and then the test will pass again without changes. > src/java.desktop/share/native/liblcms/LCMS.c line 765: > >> 763: // The profile we used for sanity checking needs to be returned >> 764: // since the one we updated is raw - not cooked. >> 765: cmsCloseProfile(p); > > Looks like now we close this profile in all branches, so can we do that just > above the "if (pfSanity == NULL)"? Ok ------------- PR: https://git.openjdk.java.net/jdk/pull/7668
