SOLR-16628: Ensure that InputStreams are closed after Xml parsing #1302

magibney · 2023-01-18T22:11:26Z

dsmiley · 2023-01-18T22:42:10Z

solr/core/src/java/org/apache/solr/core/SolrConfig.java

+    InputStream in = null;
    try {
-      ResourceProvider rp = new ResourceProvider(loader, name);
+      in = loader.openResource(name);


Can we use try-with-resources pattern here?

I think we cannot, unfortunately; because in most cases the InputStream should have been closed properly by the parser before reaching our finally block, so we're really relying on the "exception-swallowing" quality of IOUtils.closeQuietly().

Not sure if we need that. I was able to apply your change here to 9.1 in my reproducing setup. Your change, as well as a tweak to use the simpler try-with-resources both yielded working test runs.

I think IndexSchemaFactory#loadConfig has the same pattern here? at least using loader.openResource(name) without closing it?

https://github.com/apache/solr/pull/987/files#diff-5965edf45e1dec49fea047168b81ddffbb530f5db8875b41627950773d63b3a6R138

I went ahead and used try-with-resources (b648786) where possible (it's not possible in contexts where the InputStream is opened in less straightforward ways).

@risdenk in cd4923f I also addressed the case you identified in IndexSchemaFactory, and followed up the stack trace to address a similar issue in ManagedIndexSchemaFactory.

As a consequence of the ManagedIndexSchemaFactory change, this PR's a bit easier to review ignoring whitespace: https://github.com/apache/solr/pull/1302/files?w=1

solr/core/src/java/org/apache/solr/core/SolrConfig.java

dsmiley · 2023-01-19T03:05:59Z

I found a reproducible line on branch_9_1 using JDK 11 on my aging laptop.

gradlew -p solr/core test -Ptests.iters=10 --tests TestCoreContainer

Applying the changes here made this pass! Also with simpler try-with-resources pattern in one file.
Thanks again.

solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java

magibney · 2023-01-19T17:50:34Z

I tried reproducing this by stressing my system, but haven't been able to, FYI.

Separately, is the value/risk ratio of this change enough to warrant inclusion in an RC3 for 9.1.1?

dsmiley · 2023-01-19T18:18:42Z

RE 9.1.1 eh... I wouldn't bother, personally. Let's see if a more serious concern is brought up.

solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java

magibney · 2023-01-19T18:25:47Z

RE 9.1.1 eh... I wouldn't bother, personally. Let's see if a more serious concern is brought up.

This makes sense to me. I suspect this has been a (mostly) latent issue for a while now. SOLR-15337 may have made it more apparent in some ways, but I think this type of issue existed, even in this part of the codebase, even before that.

magibney · 2023-01-19T18:31:29Z

Ok, so targeting 9.2 release. Does this warrant a CHANGES.txt entry? I'm thinking "no" ... nobody has actually reported this being a problem in the wild, afaik?

solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java

dsmiley · 2023-01-19T18:57:44Z

CHANGES.txt -- I recommend adding an entry. It is a bug, albeit minor.
I recommend waiting until after 9.1.1 vote passes before merging or adding CHANGES.txt. There is a chance we'll pull it into 9.1.1 even though at this moment we think not.

solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java

magibney · 2023-01-26T15:20:14Z

With 9.1.1 released, added a CHANGES.txt entry for 9.2.0 release. Pending any feedback, I plan to merge this tomorrow.

solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java

risdenk · 2023-01-28T16:12:40Z

solr/core/build.gradle


  implementation 'commons-cli:commons-cli'

+  implementation 'net.jcip:jcip-annotations'


So maybe @dsmiley knows which jcip-annotations is correct.

At least based on https://github.com/stephenc/jcip-annotations - it should be https://mvnrepository.com/artifact/com.github.stephenc.jcip/jcip-annotations/1.0-1

however it looks like we include net.jcip:jcip-annotations already somehow?

These two JARs are equivalent but have different licenses. We should prefer stephenc as it's ASL licensed; a clean-room written implementation. The net.jcip https://jcip.net/annotations/doc/index.html is using Creative Commons Attribution License (http://creativecommons.org/licenses/by/2.5) which the ASF is okay with in binary form https://www.apache.org/legal/resolved.html#cc-by but lets prefer the ASL one as we have a choice.

I removed net.jcip in #1313 this weekend; pending merge.

@magibney lets remove the use of such annotations in this PR. In #1313 I added it.

👍 thanks for catching this, and for the work on #1313. I'll wait til after that's merged to merge this.

risdenk · 2023-02-06T15:22:45Z

@magibney #1313 was merged so this should be good to go resolving any conflicts :D

sonatype-lift · 2023-02-08T04:40:51Z

solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java

+
+      int schemaZkVersion = -1;
+      if (!(loader instanceof ZkSolrResourceLoader)) {
+        Entry<String, InputStream> localSchemaInput = readSchemaLocally();


THREAD_SAFETY_VIOLATION: Unprotected write. Non-private method ManagedIndexSchemaFactory.create(...) indirectly writes to field this.managedSchemaResourceName outside of synchronization.
Reporting because this access may occur on a background thread.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

It sucks to still see this because we have a NotThreadSafe annotation, thus I wasn't anticipating the checks. From reading the Infer/RacerD docs in more detail, it appears this is because this class, despite not being ThreadSafe, and advertising itself as such nonetheless contains the "synchronized" keyword in one spot at the end of create(). The presence of the keyword in the code takes precedence over NotThreadSafe annotation. It seems to me we don't need that synchronized there because the schema is not published yet. Any way, such changes / explorations don't belong here. I'll create a PR to toy with this.

Ah, ok, that makes sense! Thanks for following up on this. I expect to merge/backport tomorrow.

) (cherry picked from commit e20fcf8)

magibney added 2 commits January 18, 2023 17:06

SOLR-16628: Ensure that InputStreams are closed after Xml parsing

974c749

close InputStream within XmlConfigFile ctor as well (instead?)

f198643

dsmiley reviewed Jan 18, 2023

View reviewed changes

solr/core/src/java/org/apache/solr/core/SolrConfig.java Outdated Show resolved Hide resolved

sonatype-lift bot reviewed Jan 18, 2023

View reviewed changes

solr/core/src/java/org/apache/solr/core/SolrConfig.java Show resolved Hide resolved

magibney added 4 commits January 19, 2023 10:00

use try-with-resources where possible

b648786

prefer a more succinct comment

9507e1a

ensure schema InputStream is always closed

cd4923f

spotlessApply fix formatting issue

f12517d

sonatype-lift bot reviewed Jan 19, 2023

View reviewed changes

solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java Show resolved Hide resolved

dsmiley approved these changes Jan 19, 2023

View reviewed changes

loadedResource can and should be a local, not instance, variable

3a047d4

dsmiley approved these changes Jan 19, 2023

View reviewed changes

solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java Show resolved Hide resolved

sonatype-lift bot reviewed Jan 19, 2023

View reviewed changes

solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java Outdated Show resolved Hide resolved

magibney added 2 commits January 19, 2023 14:26

annotate ManagedIndexSchemaFactory class as NotThreadSafe

a14fc44

reformat imports with spotlessApply

3ee812e

sonatype-lift bot reviewed Jan 19, 2023

View reviewed changes

solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java Show resolved Hide resolved

magibney added 2 commits January 26, 2023 09:59

Merge remote-tracking branch 'apache-solr/main' into SOLR-16628

1653dcd

add CHANGES.txt entry for 9.2.0

6216132

risdenk reviewed Jan 26, 2023

View reviewed changes

solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java Outdated Show resolved Hide resolved

avoid passing schema resource by array arg side-effect

39c5620

risdenk requested changes Jan 28, 2023

View reviewed changes

Merge remote-tracking branch 'apache-solr/main' into SOLR-16628

75fe9a0

sonatype-lift bot reviewed Feb 8, 2023

View reviewed changes

dsmiley approved these changes Feb 8, 2023

View reviewed changes

Merge remote-tracking branch 'apache-solr/main' into SOLR-16628

ad2bca6

magibney merged commit e20fcf8 into apache:main Feb 9, 2023

magibney added a commit that referenced this pull request Feb 9, 2023

SOLR-16628: Ensure that InputStreams are closed after Xml parsing (#1302

aef81de

) (cherry picked from commit e20fcf8)


		implementation 'commons-cli:commons-cli'

		implementation 'net.jcip:jcip-annotations'

Command	Usage
`@sonatype-lift ignore`	Leave out the above finding from this PR
`@sonatype-lift ignoreall`	Leave out all the existing findings from this PR
`@sonatype-lift exclude <file\|issue\|path\|tool>`	Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file

SOLR-16628: Ensure that InputStreams are closed after Xml parsing #1302

SOLR-16628: Ensure that InputStreams are closed after Xml parsing #1302

Uh oh!

Conversation

magibney commented Jan 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dsmiley commented Jan 19, 2023

Uh oh!

Uh oh!

magibney commented Jan 19, 2023

Uh oh!

dsmiley commented Jan 19, 2023

Uh oh!

Uh oh!

magibney commented Jan 19, 2023

Uh oh!

magibney commented Jan 19, 2023

Uh oh!

Uh oh!

dsmiley commented Jan 19, 2023

Uh oh!

Uh oh!

magibney commented Jan 26, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

risdenk commented Feb 6, 2023

Uh oh!

sonatype-lift bot Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!