Time format in jar and jarsigner output (was Re: Code review request: 7163483 JarSigner -verify -verbose does not format date string according to locale)

Wed Jul 11 06:21:18 UTC 2012

Mostly fine.

1. "import java.text.FieldPosition;" is not needed anymore.

2. In jar/Main, you can also consider creating a separate 
SimpleDateFormat object (maybe private final static?). It seems the 
initialization is quite heavy.

You recently became a jdk8 committer, right? That means you can do your 
own push but if you want to make any changes to the bug database, just 
tell me.

Thanks
Max

On 07/11/2012 02:07 PM, Jonathan Lu wrote:
> Hello Max,
>
> Thanks a lot for review, here's the updated patch,
>
> http://cr.openjdk.java.net/~luchsh/7163483_5/
>
> On 07/10/2012 08:54 AM, Weijun Wang wrote:
>> Hi Jonathon
>>
>> This is better. Two minor comments, you can decide what to do or we
>> can see what core-lib-devs think:
>
> I was proposing my solution for this issue and I'd like to hear more
> from core-lib-devs list. :)
>
>>
>> 1. Current output uses "zzz" for timezones, you're using "ZZZ". It
>> might be more of an ISO flavor.
>
> Changed to "zzz" format which seems to be more readable for me.
>
>>
>> 2. The DateFormat.format(Date,StringBuffer,FilePosition) is quite
>> advanced. If it's me, I'll use the simpler format(Data) method.
>
> Updated in the new patch.
>
>>
>> Since the jar command also uses the same output format, can you also
>> make the same change there (that's sun.tools.jar.Main) and ask
>> core-libs-dev at openjdk.java.net for a review? Sherman
>> (xueming.shen at oracle.com) owns the jar tool.
> I've included sun.tools.jar.Main in the latest patch and CCed Sherman.
>
> Could you please help to take another look?
>
> Many thanks
>
> Jonathan
>
>>
>> Thanks
>> Max
>>
>>
>> On 07/09/2012 03:24 PM, Jonathan Lu wrote:
>>> Hi Max,
>>>
>>> Thanks for reviewing.
>>>
>>> On 07/06/2012 06:27 PM, Weijun Wang wrote:
>>>> Hi Jonathan
>>>>
>>>> I have these questions:
>>>>
>>>> 1. Why always CAPITAL letters for month and weekday?
>>>
>>> This is a fault of the patch, which can be easily fixed by updating the
>>> format string.
>>>
>>>>
>>>> 2. Why always "dd HH:mm:ss zzz yyyy" for the rest? Some locales uses
>>>> "." instead of ":" as times delimiters.
>>>> 3. Why always dd after MMM? Some locales prefer dd before MMM.
>>>
>>> For question #2 and #3, I was just trying to follow the original format
>>> of Date.toString().
>>>
>>>>
>>>> Well, if you really think the current "Fri Jul" output is too English,
>>>> instead of localizing the string, how about we de-localize it totally
>>>> and choose a neutral format?
>>>>
>>>> There are several flavors of ISO date/time format at
>>>>
>>>>   http://www.w3.org/TR/NOTE-datetime
>>>>
>>>> or we can just choose
>>>>
>>>>   YYYY-MM-dd HH:mm:ss zzz
>>>
>>> Good idea, how about a patch in this way?
>>> http://cr.openjdk.java.net/~luchsh/7163483_4/
>>>
>>> And I prefer to your format since it looks more readable to me.
>>>
>>> Thanks and best regards!
>>> Jonathan
>>>
>>>>
>>>> BTW, the jar command is using the same format, therefore I'm adding
>>>> core-libs-dev.
>>>>
>>>> Thanks
>>>> Max
>>>>
>>>>
>>>> On 07/06/2012 05:16 PM, Jonathan Lu wrote:
>>>>> Hello Max,
>>>>>
>>>>> I's been a long time since my last mail, I did some investigation and
>>>>> had some discussion with i18n developers,  but still did not see a
>>>>> nice
>>>>> solution for the alignment problem. There does not seem be an existing
>>>>> API to do this job in JDK scope. So I implemented a simple format
>>>>> function, and use it to format under different locales.
>>>>>
>>>>> http://cr.openjdk.java.net/~luchsh/7163483_3/
>>>>>
>>>>> The patch is trying to format the code in the same way as
>>>>> java.util.Date.toString() in the format of "EEE MMM dd HH:mm:ss zzz
>>>>> yyyy", except for using names of month and DOW in localized format. So
>>>>> far, it works good for me under all supported locales.
>>>>>
>>>>> Here's a test case to verify the vertical alignment, which I has been
>>>>> posted to i18n mailing list before,
>>>>> http://cr.openjdk.java.net/~luchsh/VerticalAlignmentTest.java
>>>>>
>>>>> It may still fail under "vi_VN" locale with this solution due to test
>>>>> case limit, but I do not think it is a real failure since the result
>>>>> fields still get aligned except for multiple words in one field.
>>>>>
>>>>> Could you please take a look at the patch?
>>>>>
>>>>> Many thanks & best regards
>>>>> Jonathan
>>>>>
>>>>> On 04/25/2012 07:48 PM, Weijun Wang wrote:
>>>>>> Hi Jonathan
>>>>>>
>>>>>> I'm using English.
>>>>>>
>>>>>> In your test all the files have a similar modified time so you cannot
>>>>>> see the difference. However, in my example, you can see that the
>>>>>> widths for date and hour are not zero-padded so the width can be
>>>>>> either 1 or 2.
>>>>>>
>>>>>> French is even worse
>>>>>>
>>>>>> smk       76 10 nov. 2009 08:57:54 bin/vbin/go
>>>>>> smk     1149 8 avr. 2012 16:03:20 bin/vbin/netbeans
>>>>>> smk      170 20 nov. 2009 16:47:42 bin/vbin/syncdown
>>>>>> smk      671 8 févr. 2012 20:11:22 bin/vbin/ssh.desktop
>>>>>> smk      187 20 nov. 2009 16:47:34 bin/vbin/syncsf
>>>>>>
>>>>>> So here even the width of month abbr can be different.
>>>>>>
>>>>>> Thanks
>>>>>> Max
>>>>>>
>>>>>>
>>>>>> On 04/25/2012 07:09 PM, Jonathan Lu wrote:
>>>>>>> Hello Max,
>>>>>>>
>>>>>>> Terribly sorry for my misunderstanding!
>>>>>>>
>>>>>>> On 04/25/2012 05:39 PM, Weijun Wang wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 04/25/2012 05:23 PM, Jonathan Lu wrote:
>>>>>>>>> Hi Max,
>>>>>>>>>
>>>>>>>>> On 04/25/2012 05:12 PM, Weijun Wang wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 04/25/2012 03:28 PM, Jonathan Lu wrote:
>>>>>>>>>>> Hi Weijun,
>>>>>>>>>>>
>>>>>>>>>>> Thanks for your time, I've updated the webrev, could you please
>>>>>>>>>>> take a
>>>>>>>>>>> look?
>>>>>>>>>>> http://cr.openjdk.java.net/~luchsh/7163483_2/
>>>>>>>>>>>
>>>>>>>>>>> On 04/24/2012 03:06 PM, Weijun Wang wrote:
>>>>>>>>>>>> Hi Jonathan
>>>>>>>>>>>>
>>>>>>>>>>>> Some comments:
>>>>>>>>>>>>
>>>>>>>>>>>> 1. Can you be sure that the new format always has the same
>>>>>>>>>>>> length?
>>>>>>>>>>>> jarsigner tries to output in a tabular style and each column
>>>>>>>>>>>> should be
>>>>>>>>>>>> aligned.
>>>>>>>>>>>
>>>>>>>>>>> I'm not sure of that, so the test case was updated to compare
>>>>>>>>>>> the
>>>>>>>>>>> first
>>>>>>>>>>> several tokens to determine whether there's any differences
>>>>>>>>>>> in the
>>>>>>>>>>> expression of date time.
>>>>>>>>>>
>>>>>>>>>> Sorry, I didn't make myself clear last time, I was mainly
>>>>>>>>>> afraid of
>>>>>>>>>> unaligned lines that make the output ugly.
>>>>>>>>>>
>>>>>>>>>> For example:
>>>>>>>>>>
>>>>>>>>>> smk 76 Nov 10, 2009 8:57:54 AM bin/vbin/go
>>>>>>>>>> smk 1149 Apr 8, 2012 4:03:20 PM bin/vbin/netbeans
>>>>>>>>>> smk 170 Nov 20, 2009 4:47:42 PM bin/vbin/syncdown
>>>>>>>>>> smk 671 Feb 8, 2012 8:11:22 PM bin/vbin/ssh.desktop
>>>>>>>>>> smk 187 Nov 20, 2009 4:47:34 PM bin/vbin/syncsf
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I think that would not be a problem in the new test case which
>>>>>>>>> compares
>>>>>>>>> tokenized strings splited by blank spaces instead of
>>>>>>>>> String#equals.
>>>>>>>>> Does
>>>>>>>>> that make sense?
>>>>>>>>
>>>>>>>> I'm not talking about the test. It's the output of jarsigner
>>>>>>>> looking
>>>>>>>> ugly.
>>>>>>>>
>>>>>>>> smk 76 Nov 10, 2009 8:57:54 AM bin/vbin/go
>>>>>>>> smk 1149 Apr 8, 2012 4:03:20 PM bin/vbin/netbeans
>>>>>>>> smk 170 Nov 20, 2009 4:47:42 PM bin/vbin/syncdown
>>>>>>>> smk 671 Feb 8, 2012 8:11:22 PM bin/vbin/ssh.desktop
>>>>>>>> smk 187 Nov 20, 2009 4:47:34 PM bin/vbin/syncsf
>>>>>>>>
>>>>>>>> Compare with the current output:
>>>>>>>>
>>>>>>>> smk 76 Tue Nov 10 08:57:54 CST 2009 bin/vbin/go
>>>>>>>> smk 1149 Sun Apr 08 16:03:20 CST 2012 bin/vbin/netbeans
>>>>>>>> smk 170 Fri Nov 20 16:47:42 CST 2009 bin/vbin/syncdown
>>>>>>>> smk 671 Wed Feb 08 20:11:22 CST 2012 bin/vbin/ssh.desktop
>>>>>>>> smk 187 Fri Nov 20 16:47:34 CST 2009 bin/vbin/syncsf
>>>>>>>
>>>>>>> I did not see unaligned format in my testing, did you get these
>>>>>>> unaligned output after applying the patch? From above lines, I
>>>>>>> see the
>>>>>>> starting indices of date string in each line are always the same,
>>>>>>> which
>>>>>>> is achieved by jarsigner, but the length of the date strings are
>>>>>>> not the
>>>>>>> same, which locale were you testing on?
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Max
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>> Max
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> 2. You might need to reformat the modified line to make it fit
>>>>>>>>>>>> into 80
>>>>>>>>>>>> characters width.
>>>>>>>>>>>>
>>>>>>>>>>>> 3. Why not include the test inside the changeset?
>>>>>>>>>>> 2, 3 were done in the new patch
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks
>>>>>>>>>>>> Max
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 04/23/2012 05:46 PM, Jonathan Lu wrote:
>>>>>>>>>>>>> Hello security-dev,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Here's a patch for bug 7163483, could anybody please help to
>>>>>>>>>>>>> take a
>>>>>>>>>>>>> look?
>>>>>>>>>>>>> http://cr.openjdk.java.net/~luchsh/7163483/
>>>>>>>>>>>>>
>>>>>>>>>>>>> The problem is that command "jarsigner -verify -verbose
>>>>>>>>>>>>> my.jar"
>>>>>>>>>>>>> does not
>>>>>>>>>>>>> format date string according to current locale. following
>>>>>>>>>>>>> simple
>>>>>>>>>>>>> test
>>>>>>>>>>>>> case can be used to disclose this problem.
>>>>>>>>>>>>>
>>>>>>>>>>>>> /*
>>>>>>>>>>>>> * Copyright (c) 2012 Oracle and/or its affiliates. All rights
>>>>>>>>>>>>> reserved.
>>>>>>>>>>>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE
>>>>>>>>>>>>> HEADER.
>>>>>>>>>>>>> *
>>>>>>>>>>>>> * This code is free software; you can redistribute it and/or
>>>>>>>>>>>>> modify it
>>>>>>>>>>>>> * under the terms of the GNU General Public License version 2
>>>>>>>>>>>>> only, as
>>>>>>>>>>>>> * published by the Free Software Foundation.
>>>>>>>>>>>>> *
>>>>>>>>>>>>> * This code is distributed in the hope that it will be
>>>>>>>>>>>>> useful, but
>>>>>>>>>>>>> WITHOUT
>>>>>>>>>>>>> * ANY WARRANTY; without even the implied warranty of
>>>>>>>>>>>>> MERCHANTABILITY or
>>>>>>>>>>>>> * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
>>>>>>>>>>>>> License
>>>>>>>>>>>>> * version 2 for more details (a copy is included in the
>>>>>>>>>>>>> LICENSE
>>>>>>>>>>>>> file
>>>>>>>>>>>>> that
>>>>>>>>>>>>> * accompanied this code).
>>>>>>>>>>>>> *
>>>>>>>>>>>>> * You should have received a copy of the GNU General Public
>>>>>>>>>>>>> License
>>>>>>>>>>>>> version
>>>>>>>>>>>>> * 2 along with this work; if not, write to the Free Software
>>>>>>>>>>>>> Foundation,
>>>>>>>>>>>>> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301
>>>>>>>>>>>>> USA.
>>>>>>>>>>>>> *
>>>>>>>>>>>>> * Please contact Oracle, 500 Oracle Parkway, Redwood
>>>>>>>>>>>>> Shores, CA
>>>>>>>>>>>>> 94065
>>>>>>>>>>>>> USA
>>>>>>>>>>>>> * or visit www.oracle.com if you need additional
>>>>>>>>>>>>> information or
>>>>>>>>>>>>> have any
>>>>>>>>>>>>> * questions.
>>>>>>>>>>>>> */
>>>>>>>>>>>>>
>>>>>>>>>>>>> /*
>>>>>>>>>>>>> * Portions Copyright (c) 2012 IBM Corporation
>>>>>>>>>>>>> */
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> import java.io.ByteArrayOutputStream;
>>>>>>>>>>>>> import java.io.PrintStream;
>>>>>>>>>>>>> import java.util.Locale;
>>>>>>>>>>>>> import sun.security.tools.JarSigner;
>>>>>>>>>>>>>
>>>>>>>>>>>>> public class bug7163483 {
>>>>>>>>>>>>>
>>>>>>>>>>>>> public static void main(String[] args) throws Exception {
>>>>>>>>>>>>> final String[] arg = { "-verify", "-verbose",
>>>>>>>>>>>>> System.getProperty("java.home")+"/lib/jce.jar"};
>>>>>>>>>>>>>
>>>>>>>>>>>>> ByteArrayOutputStream stream = new
>>>>>>>>>>>>> ByteArrayOutputStream(1024*64);
>>>>>>>>>>>>> PrintStream out = new PrintStream(stream);
>>>>>>>>>>>>> System.setOut(out);
>>>>>>>>>>>>>
>>>>>>>>>>>>> Locale.setDefault(Locale.GERMAN);
>>>>>>>>>>>>> JarSigner js = new JarSigner();
>>>>>>>>>>>>> js.run(arg);
>>>>>>>>>>>>>
>>>>>>>>>>>>> out.flush();
>>>>>>>>>>>>> String s1 = stream.toString();
>>>>>>>>>>>>> s1 = s1.substring(0, s1.length()/2);
>>>>>>>>>>>>> stream.reset();
>>>>>>>>>>>>>
>>>>>>>>>>>>> Locale.setDefault(Locale.FRANCE);
>>>>>>>>>>>>> js = new JarSigner();
>>>>>>>>>>>>> js.run(arg);
>>>>>>>>>>>>>
>>>>>>>>>>>>> out.flush();
>>>>>>>>>>>>> String s2 = stream.toString();
>>>>>>>>>>>>> s2 = s2.substring(0, s2.length()/2);
>>>>>>>>>>>>>
>>>>>>>>>>>>> if (s1.equals(s2)) {
>>>>>>>>>>>>> System.err.println("Header output for GERMAN locale is:"+s1);
>>>>>>>>>>>>> System.err.println("Header output for FRANCE locale is:"+s2);
>>>>>>>>>>>>> throw new RuntimeException(
>>>>>>>>>>>>> "JarSigner verbose outputs are the same after setting
>>>>>>>>>>>>> locale!!");
>>>>>>>>>>>>> } else {
>>>>>>>>>>>>> System.err.println("Header output for GERMAN locale is:"+s1);
>>>>>>>>>>>>> System.err.println("Header output for FRANCE locale is:"+s2);
>>>>>>>>>>>>> System.err.println("Test passed!");
>>>>>>>>>>>>> }
>>>>>>>>>>>>> }
>>>>>>>>>>>>> }
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks and best regards!
>>>>>>>>>>>>> - Jonathan Lu
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Best regards!
>>>>>>>>>>> - Jonathan
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>> Thanks & regards!
>>>>>>>>> - Jonathan
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> Thanks
>>>>>>> - Jonathan
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>