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.

Sign up for GitHub

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

Jump to bottom

[BUG] NPE in org.exist.xquery.functions.securitymanager.IdFunction #4670

Open
nverwer opened this issue Jan 9, 2023 · 4 comments · May be fixed by #5158
Open

[BUG] NPE in org.exist.xquery.functions.securitymanager.IdFunction #4670

nverwer opened this issue Jan 9, 2023 · 4 comments · May be fixed by #5158

Comments

@nverwer
Copy link

nverwer commented Jan 9, 2023

Describe the bug
In /exist-core/src/main/java/org/exist/xquery/functions/securitymanager/IdFunction.java an NPE can occur when the sm:id() function is called. This happens when context.getRealUser() returns null.

Expected behavior
When context.getRealUser() returns null, the <sm:real> element should be empty, or absent.

To Reproduce
This happens when an XQuery script is called from a web application, after eXist has been restarted. However, I cannot come up with a simple reliable example.

Context (please always complete the following information):

  • OS: Windows
  • eXist-db version: 6.1
  • Java Version 8 and 11

Additional context
The NPE occurs in

    private void subjectToXml(final MemTreeBuilder builder, final Subject subject) {
        builder.startElement(new QName("username", SecurityManagerModule.NAMESPACE_URI, SecurityManagerModule.PREFIX), null);
        builder.characters(subject.getName());
    ...
  }

Here, subject may be null, since is is obtained from context.getRealUser(), which may be null.

Proposed fix
I will make a PR, with a proposed solution avoiding the NPE.
nverwer pushed a commit to nverwer/exist that referenced this issue Jan 9, 2023
fix for [BUG] NPE in org.exist.xquery.functions.securitymanager.IdFun…
9111736 
…ction eXist-db#4670
@nverwer nverwer mentioned this issue Jan 9, 2023
Open
@adamretter
Copy link
Member

@nverwer Thanks for this, I appreciate the sentiment. However, the issue you describe indicates a bug at a lower-level in eXist-db as the realUser in the XQueryContext should never be null.

Unfortunately this PR would simply hide that bug. I think hiding such bugs is not really what we should do, and so I am reluctant to merge this PR.

Perhaps it would be worth looking at all sites where an XQueryContext is initialised or reused, and make sure that XQueryContext#setRealUser is called at those sites?

@nverwer
Copy link
Author

nverwer commented Jan 10, 2023

@adamretter

looking at all sites where an XQueryContext is initialised or reused
That would be feasible if I would have a way to remote debug eXist with JDWP/JDB, but I never found a way to get that working.

There is no indication in XQueryContext.java that realUser should not be null, so a bug like this is likely to occur.
I will just surround calls to sm:id() with try - catch from now on...

@nverwer nverwer closed this as completed Jan 10, 2023
@line-o
Copy link
Member

line-o commented Jan 10, 2023

We should always guard against NPEs. To not hide bugs we could throw a more meaningful error.
Obviously it was possible to trigger this.

@PieterLamers
Copy link

Should not we reopen this so it has a bit more visibility? 'completed' does not sound quite right. Is a solution like this possible? @adamretter

@adamretter adamretter reopened this Jan 16, 2023
nverwer pushed a commit to nverwer/exist that referenced this issue Sep 15, 2023
fix for [BUG] NPE in org.exist.xquery.functions.securitymanager.IdFun…
cc8dba6 
…ction eXist-db#4670
line-o pushed a commit to line-o/exist that referenced this issue Dec 11, 2023
@line-o
[bugfix] guard against NPE in securitymanager
39c00dc 
fixes eXist-db#4670
@line-o line-o linked a pull request Dec 11, 2023 that will close this issue
Open
line-o pushed a commit to line-o/exist that referenced this issue Dec 11, 2023
@line-o
[bugfix] guard against NPE in securitymanager
3af9c32 
fixes eXist-db#4670
line-o pushed a commit to line-o/exist that referenced this issue Dec 12, 2023
@line-o
[bugfix] guard against NPE in securitymanager
0adb4aa 
fixes eXist-db#4670
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[bugfix] guard against NPE in securitymanager line-o/exist
4 participants
@line-o @nverwer @adamretter @PieterLamers