[
https://issues.apache.org/jira/browse/JDO-861?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18091001#comment-18091001
]
Tilmann Zäschke commented on JDO-861:
-------------------------------------
Item 6 is a false positive. While the analysis is basically correct, there are
only 10 or so identity types and they are difficult to modify to create the
same hashcode. So this issue cannot cause more than 10 or so entries in any
hash bucket.
> List of bug reports
> -------------------
>
> Key: JDO-861
> URL: https://issues.apache.org/jira/browse/JDO-861
> Project: JDO
> Issue Type: Bug
> Components: api
> Affects Versions: JDO 3.2.1
> Reporter: Tilmann Zäschke
> Priority: Major
>
> | # | Severity | File | Issue |
> |---|----------|------|-------|
> | 1 | Bug | `identity/IntIdentity.java:131` | `compareTo` integer subtraction
> overflows |
> | 2 | Bug / Security | `LegacyJava.java:112` | `checkPermission` invokes
> instance method with `null` receiver |
> | 3 | Security | `JDOHelper.java:1156` | `setExpandEntityReferences(true)`
> weakens XML hardening |
> | 4 | Code Quality | `JDOHelper.java:1593` | Deprecated `Class.newInstance()`
> |
> | 5 | Thread Safety | `JDOException.java:238` | `inPrintStackTrace` flag
> locked on wrong monitor |
> | 6 | Minor | `identity/ObjectIdentity.java:157` | `hashCode()` omits
> class-name contribution |
> ---
> h1. Finding 1 — Integer overflow in `IntIdentity.compareTo`
> **Severity:** Bug
> **File:** `api/src/main/java/javax/jdo/identity/IntIdentity.java`, line 131
> h2. Description
> The `compareTo` method computes ordering using integer subtraction:
> ```java
> return (result == 0) ? (key - o.key) : result;
> ```
> `key - o.key` overflows when the values sit at opposite extremes. For
> example, if
> `key = Integer.MIN_VALUE` and `o.key = 1`, the subtraction wraps around to
> `Integer.MAX_VALUE`, producing a positive result when the correct answer is
> negative.
> This silently corrupts the ordering of any sorted collection (`TreeSet`,
> `TreeMap`, etc.)
> that contains `IntIdentity` instances with large positive or negative keys.
> All other narrow-type identity classes (`ByteIdentity`, `CharIdentity`,
> `ShortIdentity`)
> are safe because their types are widened to `int` before subtraction, and
> `LongIdentity`
> already avoids subtraction entirely with an explicit sign-comparison.
> `IntIdentity` is the
> only class in the hierarchy that is actually broken.
> h2. Fix
> Replace the subtraction with `Integer.compare`:
> ```java
> // Before
> return (result == 0) ? (key - o.key) : result;
> // After
> return (result == 0) ? Integer.compare(key, o.key) : result;
> ```
> ---
> h1. Finding 2 — Wrong receiver in `LegacyJava.SecurityManager.checkPermission`
> **Severity:** Bug / Security
> **File:** `api/src/main/java/javax/jdo/LegacyJava.java`, line 112
> h2. Description
> The inner `SecurityManager` class stores the real Java `SecurityManager`
> instance in the
> field `sm` and looks up the `checkPermission` method reflectively. However,
> when the
> method is invoked, `null` is passed as the target instance:
> ```java
> public void checkPermission(JDOPermission permission) {
> try {
> checkPermissionMethod.invoke(null, permission); // ← should be `sm`
> } catch (IllegalAccessException | InvocationTargetException e) {
> throw new JDOFatalInternalException(e.getMessage());
> }
> }
> ```
> `java.lang.SecurityManager.checkPermission` is an *instance* method. Passing
> `null`
> as the receiver to `Method.invoke` causes a `NullPointerException` (wrapped in
> `InvocationTargetException`), which is caught and re-thrown as a
> `JDOFatalInternalException`. The security check therefore **never executes**;
> it always
> throws an exception instead.
> This affects pre-Java-17 JVMs where the `SecurityManager` is still active. On
> Java 17+
> `IS_SECURITY_DEPRECATED` is `true` and this code path is skipped entirely.
> h2. Fix
> Pass the stored `sm` reference as the receiver:
> ```java
> // Before
> checkPermissionMethod.invoke(null, permission);
> // After
> checkPermissionMethod.invoke(sm, permission);
> ```
> ---
> h1. Finding 3 — `setExpandEntityReferences(true)` weakens XML hardening
> **Severity:** Security
> **File:** `api/src/main/java/javax/jdo/JDOHelper.java`, line 1156
> h2. Description
> `getDefaultDocumentBuilderFactory` correctly disables DOCTYPE declarations to
> prevent
> XXE injection:
> ```java
> factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl";,
> true);
> ```
> However, it immediately after enables entity reference expansion:
> ```java
> factory.setExpandEntityReferences(true);
> ```
> While the `disallow-doctype-decl` feature prevents external entity injection
> via
> DOCTYPE, leaving `setExpandEntityReferences` set to `true` is contrary to
> [OWASP's secure XML processing
> recommendations](https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html)
> and partially undermines the hardening intent. Additionally,
> `disallow-doctype-decl` is an
> Apache Xerces-specific feature; if an alternative XML parser is used it will
> throw a
> `ParserConfigurationException`. While this is handled (a
> `JDOFatalUserException` is thrown),
> defence-in-depth is better served by setting entity expansion to `false`
> regardless.
> h2. Fix
> ```java
> // Before
> factory.setExpandEntityReferences(true);
> // After
> factory.setExpandEntityReferences(false);
> ```
> ---
> h1. Finding 4 — Deprecated `Class.newInstance()` in `getEnhancer`
> **Severity:** Code Quality
> **File:** `api/src/main/java/javax/jdo/JDOHelper.java`, line 1593
> h2. Description
> The enhancer is instantiated using the API deprecated since Java 9:
> ```java
> return (JDOEnhancer) enhancerClass.newInstance();
> ```
> `Class.newInstance()` has two well-known problems:
> 1. It propagates *checked* exceptions thrown by the no-arg constructor as
> unchecked
> exceptions, bypassing the compiler's exception-checking mechanism and
> making error
> handling unpredictable.
> 2. It bypasses the explicit constructor-accessibility checks that
> `getDeclaredConstructor().newInstance()` enforces.
> The existing `catch (Exception ex)` block already handles
> `ReflectiveOperationException`,
> so the replacement requires no changes to error handling.
> h2. Fix
> ```java
> // Before
> return (JDOEnhancer) enhancerClass.newInstance();
> // After
> return (JDOEnhancer) enhancerClass.getDeclaredConstructor().newInstance();
> ```
> ---
> h1. Finding 5 — Thread-safety issue with `inPrintStackTrace` in `JDOException`
> **Severity:** Thread Safety
> **File:** `api/src/main/java/javax/jdo/JDOException.java`, lines 236–278
> h2. Description
> `inPrintStackTrace` is an instance field that controls behaviour in both
> `toString()` and
> `getCause()`. It is set inside a block synchronised on the output *stream*
> `s`:
> ```java
> synchronized (s) {
> inPrintStackTrace = true;
> super.printStackTrace(s);
> // ...
> inPrintStackTrace = false;
> }
> ```
> If two threads call `printStackTrace` on the **same exception instance** with
> **different streams**, both acquire different monitors and both mutate
> `inPrintStackTrace`
> concurrently without any coordination. The result is a race condition:
> `getCause()` may
> return `null` when it should not (or vice versa), and the nested-throwable
> block in
> `toString()` may be incorrectly included or suppressed during concurrent
> printing.
> The `@SuppressWarnings("java:S2445")` annotation acknowledges that Sonar
> flags this
> pattern, but the suppression hides a genuine concurrency defect.
> h2. Fix
> Lock on the exception instance itself, keeping stream-level synchronisation
> inside:
> ```java
> @Override
> public void printStackTrace(java.io.PrintStream s) {
> int len = nested == null ? 0 : nested.length;
> synchronized (this) {
> inPrintStackTrace = true;
> try {
> super.printStackTrace(s);
> if (len > 0) {
> s.println(MSG.msg("MSG_NestedThrowablesStackTrace"));
> for (int i = 0; i < len; ++i) {
> Throwable exception = nested[i];
> if (exception != null) {
> exception.printStackTrace(s);
> }
> }
> }
> } finally {
> inPrintStackTrace = false;
> }
> }
> }
> ```
> The same change applies to the `PrintWriter` overload.
> ---
> h1. Finding 6 — `ObjectIdentity.hashCode()` omits class-name contribution
> **Severity:** Minor
> **File:** `api/src/main/java/javax/jdo/identity/ObjectIdentity.java`, line 157
> h2. Description
> Every other `SingleFieldIdentity` subclass computes its hash code as
> `hashClassName() ^ key`, stored in the inherited `hashCode` field.
> `ObjectIdentity`
> overrides `hashCode()` to return only the key object's hash:
> ```java
> @Override
> public int hashCode() {
> return keyAsObject.hashCode(); // class name not included
> }
> ```
> The `hashCode` field is still *set* correctly in the constructor as
> `hashClassName() ^ keyAsObject.hashCode()`, but is never returned. The
> practical
> consequence is that two `ObjectIdentity` instances for *different* entity
> classes but with
> the same key value always produce the same hash code, causing guaranteed
> bucket
> collisions in any `HashMap` or `HashSet` that mixes identity types. This does
> not violate
> the `equals`/`hashCode` contract (unequal objects may share a hash), but it
> degrades
> performance in collections containing identities for multiple entity types.
> h2. Fix
> Return the pre-computed field that already incorporates the class name:
> ```java
> // Before
> @Override
> public int hashCode() {
> return keyAsObject.hashCode();
> }
> // After
> @Override
> public int hashCode() {
> return hashCode; // hashClassName() ^ keyAsObject.hashCode(), set in
> constructor
> }
> ```
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
