Skip to content

[CXF-7653]:Fix NPE in ClientProxy #414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jimma wants to merge 1 commit into from
Closed

[CXF-7653]:Fix NPE in ClientProxy #414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions core/src/main/java/org/apache/cxf/endpoint/ClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,24 +657,9 @@ protected Object[] processResult(Message message,
if (ex != null) {
throw ex;
}

if (resList == null
&& oi != null && !oi.getOperationInfo().isOneWay()) {

BindingOperationInfo boi = oi;
if (boi.isUnwrapped()) {
boi = boi.getWrappedOperation();
}
if (!boi.getOutput().getMessageParts().isEmpty()) {
//we were supposed to get some output, but didn't
throw new IllegalStateException("Response message did not contain proper response data. Expected: "
+ boi.getOutput().getMessageParts().get(0).getConcreteName());
}
}
Copy link
Member

@dkulp dkulp May 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is all this code removed? If the WSDL states that there should be a response and there isn't, an exception should be thrown. The above is correct. By pushing some of this down into the ClientProxy's, anything that doesn't use the client proxies (like Camel endpoints) wouldn't have this check in place.

Copy link
Contributor Author

@jimma jimma May 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is all this code removed? If the WSDL states that there should be a response and there isn't, an exception should be thrown. The above is correct.

When I ran reproducer EmptySoapBodyTest and found the NPE is thrown from Proxy class and root cause for this NPE is a null return value for a primitive type in InvocationHandler(ClientProxy), that's why I remove this and add this check in ClientProxy. I only looked at EmptySoapBodayTest , any other reproducer I need to have a look for CXF-7653 ?

Another reason why I remove these lines is this check doesn't work for this case: for example we have a simple wsdl which generates the following operation :

public HelloResponse doSomething(HelloRequest request);

To work with Dispatch/Provider , a provider class created to simply return an empty StreamSource like:

@WebServiceProvider(
          serviceName="HelloService",
          portName="HelloPort",
          targetNamespace="http://cxf.apache.org/provider",
          wsdlLocation="WEB-INF/hello.wsdl"
  )
@BindingType(value=javax.xml.ws.soap.SOAPBinding.SOAP11HTTP_BINDING)
@ServiceMode (value=javax.xml.ws.Service.Mode.PAYLOAD)
public class HelloProvider implements Provider<Source> {
  public Source invoke(Source req)  {
      return new StreamSource();
  }
}

The client dispatch expects receiving a null value. Add this check in ClientImpl throws IllegalStateException instead of return a null result.

By pushing some of this down into the ClientProxy's, anything that doesn't use the client proxies (like Camel endpoints) wouldn't have this check in place.

For the caml endpoints without using the client proxy, it gets a null value result. Is the null value a problem for ClientCallback and something else ?

if (resList != null) {
return resList.toArray();
}

return null;
}
protected Exception getException(Exchange exchange) {
Expand Down
4 changes: 4 additions & 0 deletions rt/frontend/jaxws/src/main/java/org/apache/cxf/jaxws/JaxWsClientProxy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
} else {
result = invokeSync(method, oi, params);
}
if (result == null && !method.getReturnType().equals(Void.TYPE)
Copy link

@andrei-ivanov andrei-ivanov May 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: you can use == to check class equality

Copy link
Contributor Author

@jimma jimma May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for review. @andrei-ivanov . I agree with you, == is more direct for class equality.

&& method.getReturnType().isPrimitive()) {
throw new IllegalStateException("Response message did not contain proper response data");
}
} catch (WebServiceException wex) {
throw wex;
} catch (Exception ex) {
Expand Down
3 changes: 3 additions & 0 deletions rt/frontend/simple/src/main/java/org/apache/cxf/frontend/ClientProxy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
}

Object o = invokeSync(method, oi, params);
if (o == null && !method.getReturnType().equals(Void.TYPE) && method.getReturnType().isPrimitive()) {
Copy link

@andrei-ivanov andrei-ivanov May 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: you can use == to check class equality

Copy link
Contributor Author

@jimma jimma May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 .

throw new IllegalStateException("Response message did not contain proper response data");
}
//call a virtual method passing the object. This causes the IBM JDK
//to keep the "this" pointer references and thus "this" doesn't get
//finalized in the midst of an invoke operation
Expand Down