Re: [cp-patches] RFC: fix JarEntry cert lookup
On Sunday 26 November 2006 19:58, Casey Marshall wrote: On Nov 25, 2006, at 3:59 PM, Raif S. Naffah wrote: On Tuesday 21 November 2006 08:35, Casey Marshall wrote: ... This patch fixes an issue with JarEntry objects that are created before the entry is verified... ...before your patch our implementation outputs 0, 0, and 1, and after the patch: 1, 1, and 1 which is different in both cases. Yeah, JarEntry needs to check if the entry has been verified first before looking in the certificates map. Attached is an updated patch; with it the TestOfManifest test passes. yes. please update the copyright year of JarEntry when you check in the patch. any reason why you didn't address the synchronization issue in JarEntry#getCertificates()? cheers; rsn pgps8pL72xrZ7.pgp Description: PGP signature
Re: [cp-patches] RFC: fix JarEntry cert lookup
On Nov 27, 2006, at 12:08 AM, Raif S. Naffah wrote: On Sunday 26 November 2006 19:58, Casey Marshall wrote: On Nov 25, 2006, at 3:59 PM, Raif S. Naffah wrote: On Tuesday 21 November 2006 08:35, Casey Marshall wrote: ... This patch fixes an issue with JarEntry objects that are created before the entry is verified... ...before your patch our implementation outputs 0, 0, and 1, and after the patch: 1, 1, and 1 which is different in both cases. Yeah, JarEntry needs to check if the entry has been verified first before looking in the certificates map. Attached is an updated patch; with it the TestOfManifest test passes. yes. please update the copyright year of JarEntry when you check in the patch. OK. any reason why you didn't address the synchronization issue in JarEntry#getCertificates()? I wasn't sure if it was needed. I think when just reading the certs mapping, it doesn't need to be (that map is created when the manifest is read, and not modified after). But, the validity mapping probably needs synchronized access. I'll look at it. Thanks.
Re: [cp-patches] RFC: fix JarEntry cert lookup
On Nov 27, 2006, at 10:52 AM, Casey Marshall wrote:
On Nov 27, 2006, at 12:08 AM, Raif S. Naffah wrote:
any reason why you didn't address the synchronization issue in
JarEntry#getCertificates()?
I wasn't sure if it was needed.
It is needed, and JarFile explains that you need to lock on the
JarFile object when accessing that field.
I'm checking in this patch, thanks.
2006-11-27 Casey Marshall [EMAIL PROTECTED]
* java/util/jar/JarEntry.java (certs): removed.
(jarfile): new field.
(getCertificates): read the certificates from the containing JarFile.
* java/util/jar/JarFile.java (JarEnumeration.nextElement): don't
fill in 'certs,' fill in 'jarfile' for the entry.
(getEntry): likewise.
### Eclipse Workspace Patch 1.0
#P classpath
Index: java/util/jar/JarEntry.java
===
RCS file: /cvsroot/classpath/classpath/java/util/jar/JarEntry.java,v
retrieving revision 1.6
diff -u -r1.6 JarEntry.java
--- java/util/jar/JarEntry.java 2 Jul 2005 20:32:44 - 1.6
+++ java/util/jar/JarEntry.java 28 Nov 2006 00:02:54 -
@@ -1,5 +1,5 @@
/* JarEntry.java - Represents an entry in a jar file
- Copyright (C) 2000 Free Software Foundation, Inc.
+ Copyright (C) 2000, 2006 Free Software Foundation, Inc.
This file is part of GNU Classpath.
@@ -39,6 +39,7 @@
import java.io.IOException;
import java.security.cert.Certificate;
+import java.util.Set;
import java.util.zip.ZipEntry;
/**
@@ -60,7 +61,7 @@
// (Package local) fields
Attributes attr;
- Certificate certs[];
+ JarFile jarfile;
// Constructors
@@ -79,7 +80,7 @@
{
super(name);
attr = null;
-certs = null;
+jarfile = null;
}
/**
@@ -93,7 +94,7 @@
{
super(entry);
attr = null;
-certs = null;
+jarfile = null;
}
/**
@@ -112,7 +113,7 @@
catch (IOException _)
{
}
-certs = entry.getCertificates();
+jarfile = entry.jarfile;
}
// Methods
@@ -153,13 +154,19 @@
*/
public Certificate[] getCertificates()
{
-if (certs != null)
+if (jarfile != null)
{
- return (Certificate[])certs.clone();
- }
-else
- {
- return null;
+synchronized (jarfile)
+ {
+if (jarfile.entryCerts != null)
+ {
+Set certs = (Set) jarfile.entryCerts.get(getName());
+if (certs != null
+ jarfile.verified.get(getName()) == Boolean.TRUE)
+ return (Certificate[]) certs.toArray(new
Certificate[certs.size()]);
+ }
+ }
}
+return null;
}
}
Index: java/util/jar/JarFile.java
===
RCS file: /cvsroot/classpath/classpath/java/util/jar/JarFile.java,v
retrieving revision 1.23
diff -u -r1.23 JarFile.java
--- java/util/jar/JarFile.java 20 Nov 2006 09:28:49 - 1.23
+++ java/util/jar/JarFile.java 28 Nov 2006 00:02:55 -
@@ -382,19 +382,8 @@
}
jarfile.signaturesRead = true; // fudge it.
}
-
- // Include the certificates only if we have asserted that the
- // signatures are valid. This means the certificates will not be
- // available if the entry hasn't been read yet.
- if (jarfile.entryCerts != null
- jarfile.verified.get(zip.getName()) == Boolean.TRUE)
- {
- Set certs = (Set) jarfile.entryCerts.get(jar.getName());
- if (certs != null)
- jar.certs = (Certificate[])
- certs.toArray(new Certificate[certs.size()]);
- }
}
+ jar.jarfile = jarfile;
return jar;
}
}
@@ -439,18 +428,7 @@
}
signaturesRead = true;
}
- // See the comments in the JarEnumeration for why we do this
- // check.
- if (DEBUG)
- debug(entryCerts= + entryCerts + verified + name
- + ? + verified.get(name));
- if (entryCerts != null verified.get(name) == Boolean.TRUE)
- {
- Set certs = (Set) entryCerts.get(name);
- if (certs != null)
- jarEntry.certs = (Certificate[])
- certs.toArray(new Certificate[certs.size()]);
- }
+jarEntry.jarfile = this;
return jarEntry;
}
return null;
Re: [cp-patches] RFC: fix JarEntry cert lookup
On Nov 25, 2006, at 3:59 PM, Raif S. Naffah wrote:
hello Casey,
On Tuesday 21 November 2006 08:35, Casey Marshall wrote:
...
This patch fixes an issue with JarEntry objects that are created
before the entry is verified: we used to bind the certificates to a
JarEntry at creation time, so if the entry has not been verified yet,
the certificates field will be null. This is contrary to the JDK
behavior.
This changes JarEntry to hold a pointer of its containing JarFile,
and getCertificates now just accesses the entryCerts field of
JarFile directly. Thus, once an entry is verified, its certificates
may be looked up with a previously-initialized entry...
i've added a new method to the Mauve test in
gnu/testlet/java/util/jar/JarFile/TestOfManifest.java to highlight
this
issue.
with both RI 1.4 (1.4.2_13) and 1.5 (1.5.0_09) the behavior is the
same: the
counts of certificates is 0, 1, and 1 respectively. before your
patch our
implementation outputs 0, 0, and 1, and after the patch: 1, 1, and
1 which is
different in both cases.
Yeah, JarEntry needs to check if the entry has been verified first
before looking in the certificates map. Attached is an updated patch;
with it the TestOfManifest test passes.
### Eclipse Workspace Patch 1.0
#P classpath
Index: java/util/jar/JarEntry.java
===
RCS file: /cvsroot/classpath/classpath/java/util/jar/JarEntry.java,v
retrieving revision 1.6
diff -u -r1.6 JarEntry.java
--- java/util/jar/JarEntry.java 2 Jul 2005 20:32:44 - 1.6
+++ java/util/jar/JarEntry.java 26 Nov 2006 08:39:02 -
@@ -39,6 +39,7 @@
import java.io.IOException;
import java.security.cert.Certificate;
+import java.util.Set;
import java.util.zip.ZipEntry;
/**
@@ -60,7 +61,7 @@
// (Package local) fields
Attributes attr;
- Certificate certs[];
+ JarFile jarfile;
// Constructors
@@ -79,7 +80,7 @@
{
super(name);
attr = null;
-certs = null;
+jarfile = null;
}
/**
@@ -93,7 +94,7 @@
{
super(entry);
attr = null;
-certs = null;
+jarfile = null;
}
/**
@@ -112,7 +113,7 @@
catch (IOException _)
{
}
-certs = entry.getCertificates();
+jarfile = entry.jarfile;
}
// Methods
@@ -153,13 +154,17 @@
*/
public Certificate[] getCertificates()
{
-if (certs != null)
+if (jarfile != null)
{
- return (Certificate[])certs.clone();
- }
-else
- {
- return null;
+// XXX synchronization???
+if (jarfile.entryCerts != null)
+ {
+Set certs = (Set) jarfile.entryCerts.get(getName());
+if (certs != null
+ jarfile.verified.get(getName()) == Boolean.TRUE)
+ return (Certificate[]) certs.toArray(new
Certificate[certs.size()]);
+ }
}
+return null;
}
}
Index: java/util/jar/JarFile.java
===
RCS file: /cvsroot/classpath/classpath/java/util/jar/JarFile.java,v
retrieving revision 1.23
diff -u -r1.23 JarFile.java
--- java/util/jar/JarFile.java 20 Nov 2006 09:28:49 - 1.23
+++ java/util/jar/JarFile.java 26 Nov 2006 08:39:02 -
@@ -382,19 +382,8 @@
}
jarfile.signaturesRead = true; // fudge it.
}
-
- // Include the certificates only if we have asserted that the
- // signatures are valid. This means the certificates will not be
- // available if the entry hasn't been read yet.
- if (jarfile.entryCerts != null
- jarfile.verified.get(zip.getName()) == Boolean.TRUE)
- {
- Set certs = (Set) jarfile.entryCerts.get(jar.getName());
- if (certs != null)
- jar.certs = (Certificate[])
- certs.toArray(new Certificate[certs.size()]);
- }
}
+ jar.jarfile = jarfile;
return jar;
}
}
@@ -439,18 +428,7 @@
}
signaturesRead = true;
}
- // See the comments in the JarEnumeration for why we do this
- // check.
- if (DEBUG)
- debug(entryCerts= + entryCerts + verified + name
- + ? + verified.get(name));
- if (entryCerts != null verified.get(name) == Boolean.TRUE)
- {
- Set certs = (Set) entryCerts.get(name);
- if (certs != null)
- jarEntry.certs = (Certificate[])
- certs.toArray(new Certificate[certs.size()]);
- }
+jarEntry.jarfile = this;
return jarEntry;
}
return null;
