You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@shiro.apache.org by GitBox <gi...@apache.org> on 2020/11/12 00:40:36 UTC
[GitHub] [shiro] lcc opened a new pull request #266: Fix: flushes ByteArrayOutputStream before using
lcc opened a new pull request #266:
URL: https://github.com/apache/shiro/pull/266
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.
- [x] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [x] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lcc edited a comment on pull request #266: Fix: flushes ByteArrayOutputStream before using
Posted by GitBox <gi...@apache.org>.
lcc edited a comment on pull request #266:
URL: https://github.com/apache/shiro/pull/266#issuecomment-726929310
> 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lcc commented on pull request #266: Fix: flushes ByteArrayOutputStream before using
Posted by GitBox <gi...@apache.org>.
lcc commented on pull request #266:
URL: https://github.com/apache/shiro/pull/266#issuecomment-726351095
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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lcc commented on pull request #266: Fix: flushes ByteArrayOutputStream before using
Posted by GitBox <gi...@apache.org>.
lcc commented on pull request #266:
URL: https://github.com/apache/shiro/pull/266#issuecomment-726779759
It's a runtime code analysis tool, [javaMOP](http://fsl.cs.illinois.edu/index.php/JavaMOP4), 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(DelegatingMethodAccessorI
mpl.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.Pa
rentRunner.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.evaluateSeque
ntial(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), o
rg.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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lcc commented on pull request #266: Fix: flushes ByteArrayOutputStream before using
Posted by GitBox <gi...@apache.org>.
lcc commented on pull request #266:
URL: https://github.com/apache/shiro/pull/266#issuecomment-726929310
| 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bdemers commented on pull request #266: Fix: flushes ByteArrayOutputStream before using
Posted by GitBox <gi...@apache.org>.
bdemers commented on pull request #266:
URL: https://github.com/apache/shiro/pull/266#issuecomment-726151118
@lcc did you run into an issue with this?
`ByteArrayOutputStream.flush()` is an empty method
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bdemers merged pull request #266: Fix: flushes ByteArrayOutputStream before using
Posted by GitBox <gi...@apache.org>.
bdemers merged pull request #266:
URL: https://github.com/apache/shiro/pull/266
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bdemers commented on pull request #266: Fix: flushes ByteArrayOutputStream before using
Posted by GitBox <gi...@apache.org>.
bdemers commented on pull request #266:
URL: https://github.com/apache/shiro/pull/266#issuecomment-726824947
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lcc commented on pull request #266: Fix: flushes ByteArrayOutputStream before using
Posted by GitBox <gi...@apache.org>.
lcc commented on pull request #266:
URL: https://github.com/apache/shiro/pull/266#issuecomment-726858688
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bdemers commented on a change in pull request #266: Fix: flushes ByteArrayOutputStream before using
Posted by GitBox <gi...@apache.org>.
bdemers commented on a change in pull request #266:
URL: https://github.com/apache/shiro/pull/266#discussion_r522191623
##########
File path: lang/src/main/java/org/apache/shiro/lang/io/DefaultSerializer.java
##########
@@ -47,6 +47,7 @@
ObjectOutputStream oos = new ObjectOutputStream(bos);
oos.writeObject(o);
oos.close();
+ baos.flush();
Review comment:
whitespace issue:
```suggestion
baos.flush();
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bdemers commented on pull request #266: Fix: flushes ByteArrayOutputStream before using
Posted by GitBox <gi...@apache.org>.
bdemers commented on pull request #266:
URL: https://github.com/apache/shiro/pull/266#issuecomment-726866804
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).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bdemers commented on pull request #266: Fix: flushes ByteArrayOutputStream before using
Posted by GitBox <gi...@apache.org>.
bdemers commented on pull request #266:
URL: https://github.com/apache/shiro/pull/266#issuecomment-726483334
Can you share the result from the analyzer?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bdemers commented on pull request #266: Fix: flushes ByteArrayOutputStream before using
Posted by GitBox <gi...@apache.org>.
bdemers commented on pull request #266:
URL: https://github.com/apache/shiro/pull/266#issuecomment-728120592
Thanks @lcc!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lcc edited a comment on pull request #266: Fix: flushes ByteArrayOutputStream before using
Posted by GitBox <gi...@apache.org>.
lcc edited a comment on pull request #266:
URL: https://github.com/apache/shiro/pull/266#issuecomment-726929310
> 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org