Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes #2136 - NPE in ExpressionOperator.printCollection #2137

Merged
merged 2 commits into from
May 10, 2024

Conversation

igormukhin
Copy link

The bug is described in detail in #2136

We are looping over an array which is located in the instance field which can be set to null while lopping which is causing NPE. To fix it we are caching the value of argumentIndices by looping over an iterator of the array.

…ator.printCollection since 2.7.11

The bug is described in detail in eclipse-ee4j#2136

We are looping over an array which is located in the instance field which can be set to `null` while lopping which is causing NPE.
To fix it we are caching the value of `argumentIndices` by looping over an iterator of the array.
@igormukhin
Copy link
Author

If this PR will be approved, the same thing should be done on the 3.0.x branch

@rfelcman
Copy link
Contributor

rfelcman commented May 9, 2024

I'm asking itself how this change helps
for (int i = 0; i < this.argumentIndices.length; i++) { -> for (final int index : this.argumentIndices) {
with NPE because in case of int[] argumentIndices = null; it throws java.lang.NullPointerException: Cannot read the array length because "<local1>" is null

@igormukhin
Copy link
Author

@rfelcman yeah, this is very hard to spot.
for (final int index : this.argumentIndices) { internally creates an iterator that reference the array. So, no matter what happens with the assignment of this.argumentIndices the iterator still references the array.

Example:

class Sample {
    private int[] fields;

    public void willWork() {
        fields = new int[] {1,2,3};
        for (int n : this.fields) {
            System.out.println(n);
            this.fields = null;
        }
    }

    public void willFail() {
        fields = new int[] {1,2,3};
        for (int i = 0; i < this.fields.length; i++) {
            int n = this.fields[i];
            System.out.println(n);
            this.fields = null;
        }
    }

    public static void main(String[] args) {
        System.out.println("Will work:");
        new Sample().willWork();

        System.out.println("Will fail:");
        new Sample().willFail();
    }
}

@rfelcman
Copy link
Contributor

rfelcman commented May 9, 2024

As others mentioned NPE under stress it looks like concurrency issue not simple NPE.

@igormukhin
Copy link
Author

igormukhin commented May 9, 2024

For our project the issue does not happen under stress. It just happens sometimes.

My first take on the issue was also concurrency, but we also have logs where the issue happens surely when there is only one thread.

Also the best indication for the issue is that it was introduced in 2.7.11 where the line for (final int index : this.argumentIndices) { was replaced with for (int i = 0; i < this.argumentIndices.length; i++) {

Every reporter of the problem agrees that the bug was introduced in 2.7.11.

@igormukhin igormukhin changed the title fixes #2136 NPE in NullPointerException in ExpressionOperator.printCollection fixes #2136 - NPE in ExpressionOperator.printCollection May 9, 2024
Copy link
Contributor

@rfelcman rfelcman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just please update copyright year in the class header

@rfelcman rfelcman merged commit af60e4e into eclipse-ee4j:2.7 May 10, 2024
2 checks passed
rfelcman added a commit that referenced this pull request May 14, 2024
Fixes #2136 
Backport from #2137
---------

Signed-off-by: Radek Felcman <radek.felcman@oracle.com>
rfelcman added a commit that referenced this pull request May 14, 2024
Fixes #2136 
Backport from #2137

Signed-off-by: Radek Felcman <radek.felcman@oracle.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants