[jboss-jira] [JBoss JIRA] Commented: (JBCACHE-938) OptimisticNodeInterceptor may NPE if node cannot be created

[Elias Ross (JIRA)]

    [ http://jira.jboss.com/jira/browse/JBCACHE-938?page=comments#action_12351152 ] 
            
Elias Ross commented on JBCACHE-938:
------------------------------------

A static analysis tool revealed that it is possible that null can reach the second notify. Creating a test case for this seems difficult.

I would remove the "if" above the [1] and throw the exception and then remove the null checks below.


> OptimisticNodeInterceptor may NPE if node cannot be created
> -----------------------------------------------------------
>
>                 Key: JBCACHE-938
>                 URL: http://jira.jboss.com/jira/browse/JBCACHE-938
>             Project: JBoss Cache
>          Issue Type: Bug
>      Security Level: Public(Everyone can see) 
>    Affects Versions: 2.0.0.ALPHA2
>            Reporter: Elias Ross
>         Assigned To: Manik Surtani
>             Fix For: 2.0.0.BETA2
>
>
> ...
>          else
>          {
>             // "fail-more-silently" patch thanks to Owen Taylor - JBCACHE-767
>             if ((ctx.getOptionOverrides() == null || !ctx.getOptionOverrides().isFailSilently()) && MethodDeclarations.isPutMethod(m.getMethodId()))
>             {
>                throw new CacheException("Unable to set node version for " + getFqn(args) + ", node is null.");
>             }
>       /* [1] */
>          }
>          switch (m.getMethodId())
>          {
>             case MethodDeclarations.putDataMethodLocal_id:
>                Boolean erase = (Boolean) args[3];
>                cache.getNotifier().notifyNodeModified(fqn, true, CacheListener.ModificationType.PUT_MAP, workspaceNode == null ? null : workspaceNode.getData() , false);
>                putDataMap((Map<Object, Object>) args[2], erase, workspace, workspaceNode);
>                cache.getNotifier().notifyNodeModified(fqn, false, CacheListener.ModificationType.PUT_MAP, workspaceNode.getData(), false);
> // ^^^ NPE on workspaceNode.getData()
>                break;
> ...
> If the node fails to be created or located, the "workspaceNode" variable may be null.
> The first "? :" check checks for a null condition.
> The second does not.
> I don't believe notification should take place if there is no workspaceNode to modify.
> The solution might be to have notifyNodeModified() take a Node and not a Map. Then, this method code check for the null case.
> Though, I think in general, it's bad to have the code try and handle this edge case, as most maintainers might not expect workspaceNode to be null during the manipulation steps. Perhaps an ELSE statement could be put in [1] to abort this method gracefully?

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: http://jira.jboss.com/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira