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

Jump to bottom

Resolve potential NPE in SchemaResourceResolver #2296

Merged
merged 3 commits into from Aug 19, 2021
Merged

Resolve potential NPE in SchemaResourceResolver #2296

merged 3 commits into from Aug 19, 2021

Conversation

pepijnve
Copy link
Contributor

What problem is this PR intended to solve?

The LSResourceResolver interface specifies that the systemId parameter may be null. The SchemaResourceResolver uses this argument without checking for this which can result a NullPointerException. This patch adds an extra null check before using the parameter.

Have you included adequate test coverage?

I'm seeing this NPE in my code, but I'm afraid I haven't been able to derive a simple reproduction case yet. No test yet either as a consequence.

Does this change affect the behavior of either the C or the Java implementations?

Only the Java implementation.

@flavorjones
Copy link
Member

Hi, @pepijnve, thanks for the PR. I'd love to be able to reproduce what you're seeing -- can you help me understand where the challenge is in finding a repro? Maybe I can help!

@pepijnve
Copy link
Contributor Author

can you help me understand where the challenge is in finding a repro?

The only issue is time 😄 I simply haven't tried to isolate this just yet.

This issue occurred for me in https://github.com/asciidoctor/asciimath/blob/master/spec/parser_spec.rb#L10 when running the specs under JRuby. I'm loading the MathML schema (https://github.com/asciidoctor/asciimath/blob/master/spec/schema/mathml2/mathml2.xsd) from disk. The root schema file loads fine as an XML document, but when attempting to interpret it as a schema, the NPE happens when attempting to resolve the relative paths to the other schema files.

@pepijnve
Copy link
Contributor Author

I'm going to try loading those schemas from a standalone script to see if I can reproduce the problem and if so see if I can get the same error with a simpler schema. I'll let you know how that goes.

@pepijnve
Copy link
Contributor Author 

require 'nokogiri'

schema_file = File.expand_path('../mathml2.xsd', __FILE__)
File.open(schema_file) { |io| Nokogiri::XML::Schema(io) }

results in the following stack trace

Unhandled Java exception: java.lang.NullPointerException
java.lang.NullPointerException: null
                                               resolveResource at nokogiri/XmlSchema.java:279
                                                 resolveEntity at org/apache/xerces/util/DOMEntityResolverWrapper:-1
                                                 resolveEntity at org/apache/xerces/impl/XMLEntityManager:-1
                                               resolveDocument at org/apache/xerces/impl/xs/XMLSchemaLoader:-1
                                                 resolveSchema at org/apache/xerces/impl/xs/traversers/XSDHandler:-1
                                                constructTrees at org/apache/xerces/impl/xs/traversers/XSDHandler:-1
                                                constructTrees at org/apache/xerces/impl/xs/traversers/XSDHandler:-1
                                                   parseSchema at org/apache/xerces/impl/xs/traversers/XSDHandler:-1
                                                    loadSchema at org/apache/xerces/impl/xs/XMLSchemaLoader:-1
                                                   loadGrammar at org/apache/xerces/impl/xs/XMLSchemaLoader:-1
                                                   loadGrammar at org/apache/xerces/impl/xs/XMLSchemaLoader:-1
                                                     newSchema at org/apache/xerces/jaxp/validation/XMLSchemaFactory:-1
                                                     newSchema at javax/xml/validation/SchemaFactory.java:669
                                                     getSchema at nokogiri/XmlSchema.java:89
                                          createSchemaInstance at nokogiri/XmlSchema.java:116
                                                     getSchema at nokogiri/XmlSchema.java:186
                                                 from_document at nokogiri/XmlSchema.java:165
                                                          call at nokogiri/XmlSchema$INVOKER$s$0$1$from_document.gen:-1
                                                          call at org/jruby/internal/runtime/methods/JavaMethod.java:801
                                                          call at org/jruby/internal/runtime/methods/DynamicMethod.java:207
                                                  cacheAndCall at org/jruby/runtime/callsite/CachingCallSite.java:367
                                                          call at org/jruby/runtime/callsite/CachingCallSite.java:203
                                                   processCall at org/jruby/ir/interpreter/InterpreterEngine.java:326
                                                     interpret at org/jruby/ir/interpreter/StartupInterpreterEngine.java:72
                                                     interpret at org/jruby/ir/interpreter/InterpreterEngine.java:92
                                              INTERPRET_METHOD at org/jruby/internal/runtime/methods/MixedModeIRMethod.java:204
                                                          call at org/jruby/internal/runtime/methods/MixedModeIRMethod.java:191
                                                          call at org/jruby/internal/runtime/methods/DynamicMethod.java:207
                                                  cacheAndCall at org/jruby/runtime/callsite/CachingCallSite.java:367
                                                          call at org/jruby/runtime/callsite/CachingCallSite.java:203
                                                   processCall at org/jruby/ir/interpreter/InterpreterEngine.java:326
                                                     interpret at org/jruby/ir/interpreter/StartupInterpreterEngine.java:72
                                                     interpret at org/jruby/ir/interpreter/InterpreterEngine.java:86
                                              INTERPRET_METHOD at org/jruby/internal/runtime/methods/MixedModeIRMethod.java:171
                                                          call at org/jruby/internal/runtime/methods/MixedModeIRMethod.java:158
                                                          call at org/jruby/internal/runtime/methods/DynamicMethod.java:199
                                                  cacheAndCall at org/jruby/runtime/callsite/CachingCallSite.java:346
                                                          call at org/jruby/runtime/callsite/CachingCallSite.java:172
                                           invokeOther0:Schema at Users/pepijn/Projects/asciimath/spec/schema/mathml2//Users/pepijn/Projects/asciimath/spec/schema/mathml2/noko.rb:5
  /Users/pepijn/Projects/asciimath/spec/schema/mathml2/noko.rb at /Users/pepijn/Projects/asciimath/spec/schema/mathml2/noko.rb:5
                                                   yieldDirect at org/jruby/runtime/CompiledIRBlockBody.java:146
                                                         yield at org/jruby/runtime/BlockBody.java:114
                                                         yield at org/jruby/runtime/Block.java:165
                                              ensureYieldClose at org/jruby/RubyIO.java:1160
                                                          open at org/jruby/RubyIO.java:1154
                                                          call at org/jruby/RubyIO$INVOKER$s$0$0$open.gen:-1
                                                          call at org/jruby/internal/runtime/methods/DynamicMethod.java:203
                                                  cacheAndCall at org/jruby/runtime/callsite/CachingCallSite.java:357
                                                          call at org/jruby/runtime/callsite/CachingCallSite.java:182
                                                      callIter at org/jruby/runtime/callsite/CachingCallSite.java:189
                                             invokeOther7:open at Users/pepijn/Projects/asciimath/spec/schema/mathml2//Users/pepijn/Projects/asciimath/spec/schema/mathml2/noko.rb:5
                                                        <main> at /Users/pepijn/Projects/asciimath/spec/schema/mathml2/noko.rb:5
                                           invokeWithArguments at java/lang/invoke/MethodHandle.java:719
                                                          load at org/jruby/ir/Compiler.java:94
                                                     runScript at org/jruby/Ruby.java:849
                                                   runNormally at org/jruby/Ruby.java:772
                                                   runNormally at org/jruby/Ruby.java:790
                                                   runFromMain at org/jruby/Ruby.java:602
                                                 doRunFromMain at org/jruby/Main.java:415
                                                   internalRun at org/jruby/Main.java:307
                                                           run at org/jruby/Main.java:234
                                                          main at org/jruby/Main.java:206

I tried the same thing with a more trivial schema, but no luck. The same code works without error with MRI.

@flavorjones
Copy link
Member

@pepijnve Thanks for the more detailed information! I'll take a look this weekend.

@flavorjones flavorjones added this to the v1.13.0 milestone Aug 2, 2021
flavorjones and others added 3 commits August 18, 2021 18:56
@flavorjones
test: move the test from 3c95d44 to be within a test class
5840d84 
I don't think this test ever actually ran.
@flavorjones
test: repro for sparklemotion#2296
dc61332
@pepijnve @flavorjones
Resolve potential NPE in SchemaResourceResolver
88911fe 
The LSResourceResolver interface specifies that the systemId parameter may be null. The SchemaResourceResolver uses this argument without checking for this which can result a NullPointerException. This patch adds an extra null check before using the parameter.
@flavorjones
Copy link
Member

I've added a test that isolates this bug to the PR. Let's make sure CI goes green!

@flavorjones
Copy link
Member

Merging, and targetting v1.13 for this. Thanks again for your contribution!

@flavorjones flavorjones merged commit 10bc4e7 into sparklemotion:main Aug 19, 2021
flavorjones added a commit that referenced this pull request Aug 19, 2021
@flavorjones
update CHANGELOG for #2296
29ce8b2 
[skip ci]
@pepijnve pepijnve deleted the patch-1 branch August 19, 2021 07:54
flavorjones added a commit that referenced this pull request Aug 28, 2021
@flavorjones
test: repro for #2296
9eab65e
flavorjones added a commit that referenced this pull request Aug 28, 2021
@flavorjones
update CHANGELOG for #2296
e3f2209 
[skip ci]
flavorjones added a commit that referenced this pull request Aug 29, 2021
@flavorjones
update CHANGELOG for #2296
9432a8a 
[skip ci]
@flavorjones
Copy link
Member

This has been released as part of v1.12.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
platform/jruby
Projects
None yet
Milestone
v1.12.x patch releases
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pepijnve @flavorjones