fix NPE during compare #16
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chibenwa
reviewed
May 7, 2017
There was a problem hiding this comment.
Hi,
Thank you for this report and fixes. Do you have time to take my suggestions into account?
I'm also curious to know in wich situation you encountered this NPE... Do you have any specific script? That would allow us to write integration tests, and identify why the original String was null.
Best regards,
Benoit
| @@ -40,6 +40,8 @@ public AsciiCasemap() { | |||
| * @see org.apache.jsieve.comparators.Equals#equals(String, String) | |||
| */ | |||
| public boolean equals(String string1, String string2) { | |||
| if (string1 == null && string2 == null) return true; | |||
There was a problem hiding this comment.
You have an indentation problem, this if is not at the same level than the other one.
We also tend to always use a block after a if.
There was a problem hiding this comment.
Do you think it would be possible to you to propose tests for this changeset?
Maybe we can write a AsciiCasemapTest class, and test :
- null, null
- "a", null
- null, "a"
- "a", "b"
- "a", "a"
- "a", "A"
|
We might also need an issue on https://issues.apache.org/jira/browse/JSIEVE |
|
Hi Benoit,
I met this bug during I'm using filter in Zimbra. :-)
Zimbra(https://www.zimbra.com/) uses this library and I uses Zimbra.
Best Regards
Valery
…________________________________
От: Tellier Benoit [notifications@github.com]
Отправлено: 7 мая 2017 г. 5:28
Кому: apache/james-jsieve
Копия: pv; Author
Тема: Re: [apache/james-jsieve] fix NPE during compare (#16)
We might also need an issue on https://issues.apache.org/jira/browse/JSIEVE
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#16 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AFZJ8uDCVgIk3xO4wfvO0XKa22nZbdlGks5r3Sw0gaJpZM4NSw-S>.
|
|
Hi Benoit,
It is part of Zimbra log, maybe it is useful:
Zimbra version is 8.7.1_GA_1670
Best Regards
Valery
java.lang.NullPointerException
at
org.apache.jsieve.comparators.AsciiCasemap.equals(AsciiCasemap.java:43)
at
org.apache.jsieve.comparators.ComparatorUtils.is(ComparatorUtils.java:212)
at
org.apache.jsieve.comparators.ComparatorUtils.match(ComparatorUtils.java:63)
at org.apache.jsieve.tests.Address.match(Address.java:88)
at org.apache.jsieve.tests.Address.match(Address.java:52)
at
org.apache.jsieve.tests.AbstractComparatorTest.match(AbstractComparatorTest.java:197)
at
org.apache.jsieve.tests.AbstractComparatorTest.match(AbstractComparatorTest.java:169)
at
org.apache.jsieve.tests.AbstractComparatorTest.executeBasic(AbstractComparatorTest.java:144)
at
org.apache.jsieve.tests.AbstractTest.execute(AbstractTest.java:57)
at org.apache.jsieve.Test.isTestPassed(Test.java:66)
at org.apache.jsieve.TestList.anyTestsPass(TestList.java:112)
at org.apache.jsieve.tests.AnyOf.executeBasic(AnyOf.java:45)
at
org.apache.jsieve.tests.AbstractTest.execute(AbstractTest.java:57)
at org.apache.jsieve.Test.isTestPassed(Test.java:66)
at org.apache.jsieve.TestList.allTestsPass(TestList.java:92)
at org.apache.jsieve.commands.If.executeBasic(If.java:63)
at
org.apache.jsieve.commands.AbstractCommand.execute(AbstractCommand.java:122)
at org.apache.jsieve.Command.execute(Command.java:158)
at org.apache.jsieve.Commands.execute(Commands.java:83)
at org.apache.jsieve.Block.execute(Block.java:80)
at
org.apache.jsieve.SieveParserVisitorImpl.visit(SieveParserVisitorImpl.java:218)
at
org.apache.jsieve.parser.generated.ASTstart.jjtAccept(ASTstart.java:18)
at org.apache.jsieve.SieveFactory.evaluate(SieveFactory.java:155)
at
com.zimbra.cs.filter.RuleManager.applyRulesToExistingMessage(RuleManager.java:402)
at
com.zimbra.cs.service.mail.ApplyFilterRules.handle(ApplyFilterRules.java:150)
at com.zimbra.soap.SoapEngine.dispatchRequest(SoapEngine.java:605)
at com.zimbra.soap.SoapEngine.dispatch(SoapEngine.java:458)
at com.zimbra.soap.SoapEngine.dispatch(SoapEngine.java:271)
at com.zimbra.soap.SoapServlet.doWork(SoapServlet.java:303)
at com.zimbra.soap.SoapServlet.doPost(SoapServlet.java:213)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:707)
at
com.zimbra.cs.servlet.ZimbraServlet.service(ZimbraServlet.java:206)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:790)
at
org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:821)
at
org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1685)
at com.zimbra.cs.servlet.CsrfFilter.doFilter(CsrfFilter.java:169)
at
org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1668)
at
com.zimbra.cs.servlet.RequestStringFilter.doFilter(RequestStringFilter.java:54)
at
org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1668)
at
com.zimbra.cs.servlet.SetHeaderFilter.doFilter(SetHeaderFilter.java:59)
at
org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1668)
at
com.zimbra.cs.servlet.ETagHeaderFilter.doFilter(ETagHeaderFilter.java:47)
at
org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1668)
at
com.zimbra.cs.servlet.ContextPathBasedThreadPoolBalancerFilter.doFilter(ContextPathBasedThreadPoolBalancerFilter.java:107)
at
org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1668)
at
com.zimbra.cs.servlet.ZimbraQoSFilter.doFilter(ZimbraQoSFilter.java:116)
at
org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1668)
at
com.zimbra.cs.servlet.ZimbraInvalidLoginFilter.doFilter(ZimbraInvalidLoginFilter.java:117)
at
org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1668)
at
org.eclipse.jetty.servlets.DoSFilter.doFilterChain(DoSFilter.java:473)
at
org.eclipse.jetty.servlets.DoSFilter.doFilter(DoSFilter.java:318)
at
org.eclipse.jetty.servlets.DoSFilter.doFilter(DoSFilter.java:288)
at
org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1668)
at
org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:581)
at
org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:143)
at
org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:524)
at
org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:226)
at
org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1158)
at
org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:511)
at
org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:185)
at
org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1090)
at
org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141)
at
org.eclipse.jetty.server.handler.ContextHandlerCollection.handle(ContextHandlerCollection.java:213)
at
org.eclipse.jetty.server.handler.HandlerCollection.handle(HandlerCollection.java:109)
at
org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:119)
at
org.eclipse.jetty.rewrite.handler.RewriteHandler.handle(RewriteHandler.java:318)
at
org.eclipse.jetty.server.handler.gzip.GzipHandler.handle(GzipHandler.java:437)
at
org.eclipse.jetty.server.handler.DebugHandler.handle(DebugHandler.java:84)
at
org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:119)
at org.eclipse.jetty.server.Server.handle(Server.java:517)
at
org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:306)
at
org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:242)
at
org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:261)
at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:95)
at
org.eclipse.jetty.io.ssl.SslConnection.onFillable(SslConnection.java:192)
at
org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:261)
at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:95)
at
org.eclipse.jetty.io.SelectChannelEndPoint$2.run(SelectChannelEndPoint.java:75)
at
org.eclipse.jetty.util.thread.strategy.ExecuteProduceConsume.produceAndRun(ExecuteProduceConsume.java:213)
at
org.eclipse.jetty.util.thread.strategy.ExecuteProduceConsume.run(ExecuteProduceConsume.java:147)
at
org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:654)
at
org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:572)
at java.lang.Thread.run(Thread.java:745)
…On 05/07/2017 05:27 AM, Tellier Benoit wrote:
***@***.**** commented on this pull request.
Hi,
Thank you for this report and fixes. Do you have time to take my
suggestions into account?
I'm also curious to know in wich situation you encountered this NPE...
Do you have any specific script? That would allow us to write
integration tests, and identify why the original String was *null*.
Best regards,
Benoit
------------------------------------------------------------------------
In core/src/main/java/org/apache/jsieve/comparators/AsciiCasemap.java
<#16 (comment)>:
> @@ -40,6 +40,8 @@ public AsciiCasemap() {
* @see org.apache.jsieve.comparators.Equals#equals(String, String)
*/
public boolean equals(String string1, String string2) {
+ if (string1 == null && string2 == null) return true;
You have an indentation problem, this if is not at the same level than
the other one.
We also tend to always use a block after a *if*.
------------------------------------------------------------------------
In core/src/main/java/org/apache/jsieve/comparators/AsciiCasemap.java
<#16 (comment)>:
> @@ -40,6 +40,8 @@ public AsciiCasemap() {
* @see org.apache.jsieve.comparators.Equals#equals(String, String)
*/
public boolean equals(String string1, String string2) {
+ if (string1 == null && string2 == null) return true;
Do you think it would be possible to you to propose tests for this
changeset?
Maybe we can write a AsciiCasemapTest class, and test :
* null, null
* "a", null
* null, "a"
* "a", "b"
* "a", "a"
* "a", "A"
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#16 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFZJ8oMlQzFQk2BSge1zIsO8czpIEilUks5r3Sv8gaJpZM4NSw-S>.
|
|
Ok, thanks for context. Do you have time to add some tests and open the issue on our JIRA, or you prefer me doing it? Cheers, Benoit |
|
Hello Benoit,
Unfortunately, I don't have enough time for this job.
I prefer you doing it.
Thanks.
Valery
…________________________________
От: Tellier Benoit [notifications@github.com]
Отправлено: 11 мая 2017 г. 5:13
Кому: apache/james-jsieve
Копия: pv; Author
Тема: Re: [apache/james-jsieve] fix NPE during compare (#16)
Ok, thanks for context.
Do you have time to add some tests and open the issue on our JIRA, or you prefer me doing it?
Cheers,
Benoit
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#16 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AFZJ8tnsNadWt2QPa9PxcqlGkz9zr10mks5r4m7XgaJpZM4NSw-S>.
|
|
I found script which was the reason of the NPE:
# support@
if anyof (address :localpart :is :comparator "i;ascii-casemap" ["to"] "support",
address :localpart :is :comparator "i;ascii-casemap" ["from"] "support") {
fileinto "support";
stop;
}
…________________________________
От: Tellier Benoit [notifications@github.com]
Отправлено: 11 мая 2017 г. 5:13
Кому: apache/james-jsieve
Копия: pv; Author
Тема: Re: [apache/james-jsieve] fix NPE during compare (#16)
Ok, thanks for context.
Do you have time to add some tests and open the issue on our JIRA, or you prefer me doing it?
Cheers,
Benoit
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#16 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AFZJ8tnsNadWt2QPa9PxcqlGkz9zr10mks5r4m7XgaJpZM4NSw-S>.
|
|
Thanks. I'll try to have a look. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.