[
https://issues.apache.org/jira/browse/JDO-861?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Tilmann Zäschke updated JDO-861:
--------------------------------
Description:
| # | 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
}
```
> 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)
