Fix: flushes ByteArrayOutputStream before using

[lcc]

Hi, I would like to start by saying I am sorry for not following your community guidelines but since this patch is really tiny, I didn't think it would be necessary to create a task at Jira or follow the other procedures, if this is considered unnaceptable, do let me know and I will open this PR as specified. Thanks for the comprehension.

Flushes OutputStream before using the underlying ByteArrayOutputStream: when an OutputStream (or its subclass) instance is built on top of an underlying ByteArrayOutputStream instance, it should be flushed or closed before the underlying instance's toByteArray() is invoked. Failing to fulfill this requirement may cause toByteArray() to return incomplete contents.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
• In any other case, please file an Apache Individual Contributor License Agreement.

[bdemers]

@lcc did you run into an issue with this?
ByteArrayOutputStream.flush() is an empty method

[lcc]

Thank for the prompt response bdemers. I didn't run into any problems, I was just running a code analyzer and found what appeared to be a bug. Sorry for the useless PR, do you want me to close it?

[bdemers]

Can you share the result from the analyzer?

[lcc]

It's a runtime code analysis tool, javaMOP, and the output is basically the file and the line number of the spec violation (in this case the spec was ByteArrayOutputStream_FlushBeforeRetrieve).

ByteArrayOutputStream.flush() is an empty method yes, now that I have thought about it, one could make the point that even if the underlying implementation does nothing with the flush method its "good practice" to still call it if the class implements a behaviour at some point. You may want to flush the streams as there is nothing stopping the implementations to, in the future, have logic behind them.’

The ouput of the tool is:
1 Specification ByteArrayOutputStream_FlushBeforeRetrieve has been violated on line [org.apache.shiro.lang.io.DefaultSerializer.serialize(DefaultSerializer.java:50), org.apache.shiro.mgt.AbstractRememberMeManager.serialize(AbstractRememberMeManager.java:500), org.apache.shiro.mgt.AbstractRememberMeManager.convertPrincipalsToBytes(AbstractRememberMeManager.java:351), org.apache.shiro.mgt.AbstractRememberMeManager.rememberIdentity(AbstractRememberMeManager.java:337), org.apache.shiro.mgt.AbstractRememberMeManager.rememberIdentity(AbstractRememberMeManager.java:312), org.apache.shiro.mgt.AbstractRememberMeManager.onSuccessfulLogin(AbstractRememberMeManager.java:288), org.apache.shiro.web.mgt.CookieRememberMeManagerTest.onSuccessfulLogin(CookieRememberMeManagerTest.java:86), sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method), sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62), sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43), java.lang.reflect.Method.invoke(Method.java:498), org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59), org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12), org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56), org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17), org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306), org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100), org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366), org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103), org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63), org.junit.runners.ParentRunner$4.run(ParentRunner.java:331), org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79), org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329), org.junit.runners.ParentRunner.access$100(ParentRunner.java:66), org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293), org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306), org.junit.runners.ParentRunner.run(ParentRunner.java:413), org.junit.runner.JUnitCore.run(JUnitCore.java:137), org.junit.runner.JUnitCore.run(JUnitCore.java:115), org.junit.vintage.engine.execution.RunnerExecutor.execute(RunnerExecutor.java:43), java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184), java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193), java.util.Iterator.forEachRemaining(Iterator.java:116), java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801), java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:482), java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:472), java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151), java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174), java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234), java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:418), org.junit.vintage.engine.VintageTestEngine.executeAllChildren(VintageTestEngine.java:82), org.junit.vintage.engine.VintageTestEngine.execute(VintageTestEngine.java:73), org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:220), org.junit.platform.launcher.core.DefaultLauncher.lambda$execute$6(DefaultLauncher.java:188), org.junit.platform.launcher.core.DefaultLauncher.withInterceptedStreams(DefaultLauncher.java:202), org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:181), org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:128), org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.invokeAllTests(JUnitPlatformProvider.java:150), org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.invoke(JUnitPlatformProvider.java:124), org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:384), org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:345), org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:126), org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:418)]. Documentation for this property can be found at http://runtimeverification.com/monitor/annotated-java/__properties/html/mop/ByteArrayOutputStream_FlushBeforeRetrieve.html

[bdemers]

I'm not against the change, I'm just trying to understand the background a bit more. like are there are current JVMs that do something different (meaning I should check for this in other projects) Or if this static checking tool is something we should add to Shiro.

[lcc]

This tool is not static though, the violations are discovered at runtime when I was running shiro tests.

I'm not against the change - Awesome, this tool generates a lot of violations and only about 20% percent can be seen as "true bugs", this is one of them. What I am trying to do is assess which violations are worth correcting and finding a clever way to rank them (for example: fix these 4 issues and forget about the rest).

like are there are current JVMs that do something different (meaning I should check for this in other projects) - I am not sure what you meant by this, but if it involves using this tool, maybe I can help.

[bdemers]

Does it attach as an agent can you point me to the tool's site? (I like these kinds of tools)

As for my other comment:

like are there are current JVMs that do something different (meaning I should check for this in other projects)

Sorry, that was a bit poorly worded, I just meant. If there are other JVMs (for example projects like Android, Substrate, etc) where the implementation of ByteArrayOutputStream is different, then this issue becomes more important (for both Shiro and other projects).

[lcc]

Does it attach as an agent can you point me to the tool's site? (I like these kinds of tools)

Yes, it attaches as an agent.

Sorry, that was a bit poorly worded, I just meant. If there are other JVMs (for example projects like Android, Substrate, etc) where the implementation of ByteArrayOutputStream is different, then this issue becomes more important (for both Shiro and other projects).

That's actually really interesting, I think it would be a really good idea to take a look at this!

Requested links:
javaMOP link: http://fsl.cs.illinois.edu/index.php/JavaMOP4
github (usage and installation): https://github.com/runtimeverification/javamop

[bdemers]

Thanks @lcc!