Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 17 additions & 0 deletions src/main/java/org/metricshub/winrm/service/WinRMService.java
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
import org.apache.cxf.Bus.BusState;
import org.apache.cxf.BusFactory;
import org.apache.cxf.endpoint.Client;
import org.apache.cxf.transport.http.HTTPConduitFactory;
import org.apache.cxf.transport.http.asyncclient.AsyncHTTPConduit;
import org.apache.cxf.transport.http.asyncclient.AsyncHTTPConduitFactory;
import org.apache.cxf.transport.http.asyncclient.AsyncHTTPConduitFactory.UseAsyncPolicy;
Expand Down Expand Up @@ -302,7 +303,7 @@
} catch (final RuntimeException e) {
if (e.getCause() != null) {
final String message = e.getMessage() != null
? String.format(

Check warning on line 306 in src/main/java/org/metricshub/winrm/service/WinRMService.java

View workflow job for this annotation

GitHub Actions / spotbugs

VA_FORMAT_STRING_USES_NEWLINE

Format string should use %n rather than \n in org.metricshub.winrm.service.WinRMService.createInstance(WinRMEndpoint, long, Path, List)
Raw output
This format string includes a newline character (\n). In format strings, it is generally preferable to use %n, which will produce the platform-specific line separator. When using text blocks introduced in Java 15, use the \ escape sequence:String value = """ first line%n\ second line%n\ """;
"%s\n%s: %s",
e.getMessage(),
e.getCause().getClass().getSimpleName(),
Expand Down Expand Up @@ -352,10 +353,12 @@
}

if (cmdClient != null) {
shutdownConduitFactory(cmdClient);
cmdClient.destroy();
}

if (wqlClient != null) {
shutdownConduitFactory(wqlClient);
wqlClient.destroy();
}

Expand All @@ -365,6 +368,20 @@
}
}

/**
* Retrieves the {@link AsyncHTTPConduitFactory} registered on the given client's endpoint and calls
* {@link AsyncHTTPConduitFactory#shutdown()} on it to stop any background threads (e.g. the idle-connection
* reaper thread). This must be done before destroying the client to prevent thread leaks.
*
* @param client the CXF {@link Client} whose conduit factory should be shut down
*/
private void shutdownConduitFactory(final Client client) {
final Object factory = client.getEndpoint().getEndpointInfo().getProperty(HTTPConduitFactory.class.getName());
if (factory instanceof AsyncHTTPConduitFactory) {
((AsyncHTTPConduitFactory) factory).shutdown();
}
}

@Override
public WindowsRemoteCommandResult executeCommand(
final String command,
Expand Down Expand Up @@ -629,7 +646,7 @@
}
}
throw soapFault;
} catch (final NullPointerException e) {

Check warning on line 649 in src/main/java/org/metricshub/winrm/service/WinRMService.java

View workflow job for this annotation

GitHub Actions / spotbugs

DCN_NULLPOINTER_EXCEPTION

Do not catch NullPointerException like in org.metricshub.winrm.service.WinRMService.assertFaultCode(SOAPFaultException, String, boolean)
Raw output
According to SEI Cert rule ERR08-J [https://wiki.sei.cmu.edu/confluence/display/java/ERR08-J.+Do+not+catch+NullPointerException+or+any+of+its+ancestors] NullPointerException should not be caught. Handling NullPointerException is considered an inferior alternative to null-checking.

This non-compliant code catches a NullPointerException to see if an incoming parameter is null:


boolean hasSpace(String m) {
  try {
    String ms[] = m.split(" ");
    return names.length != 1;
  } catch (NullPointerException e) {
    return false;
  }
}


A compliant solution would use a null-check as in the following example:


boolean hasSpace(String m) {
    if (m == null) return false;
    String ms[] = m.split(" ");
    return names.length != 1;
}
throw soapFault;
}
}
Expand Down Expand Up @@ -688,30 +705,30 @@
final JAXBElement<?> jaxbElement = (JAXBElement<?>) object;

if (WSEN_ITEMS_QNAME.equals(jaxbElement.getName()) || WSMAN_ITEMS_QNAME.equals(jaxbElement.getName())) {
if (jaxbElement.isNil()) {
// No items
} else if (jaxbElement.getValue() instanceof AnyListType) {
// some items
final AnyListType itemList = (AnyListType) jaxbElement.getValue();
for (final Object item : itemList.getAny()) {
final Node node = toNode(item)
.orElseThrow(() ->
new WinRMException(
"Unsupported element of type %s in EnumerateResponse: %s",
object.getClass(),
object
)
);

items.add(node);
}
} else {
throw new WinRMException(
"Unsupported value in EnumerateResponse Items: %s of type: %s",
jaxbElement.getValue(),
jaxbElement.getValue().getClass()
);
}

Check warning on line 731 in src/main/java/org/metricshub/winrm/service/WinRMService.java

View workflow job for this annotation

GitHub Actions / PMD

Code Style EmptyControlStatement

Empty if statement
} else if (
WSEN_END_OF_SEQUENCE_QNAME.equals(jaxbElement.getName()) ||
WSMAN_END_OF_SEQUENCE_QNAME.equals(jaxbElement.getName())
Expand Down Expand Up @@ -790,19 +807,19 @@

final MixedDataType mixed = (MixedDataType) nestedElement.getValue();
for (final Object nestedItem : mixed.getContent()) {
if (nestedItem instanceof String) {
// Skip over whitespace
} else if (nestedItem instanceof Node) {
// Node's can't belong to two different documents, so we need to import it first
final Node nestedNode = document.importNode((Node) nestedItem, true);
rootElement.appendChild(nestedNode);
} else {
throw new WinRMException(
"Unsupported element of type %s in XmlFragment: %s",
nestedItem.getClass(),
nestedItem
);
}

Check warning on line 822 in src/main/java/org/metricshub/winrm/service/WinRMService.java

View workflow job for this annotation

GitHub Actions / PMD

Code Style EmptyControlStatement

Empty if statement
}
return Optional.of(rootElement);
}
Expand All @@ -825,7 +842,7 @@
public String getContextIdFrom(final EnumerationContextType context) throws WinRMException {
// The content of the EnumerationContext should contain a single string, the context id
if (context == null || context.getContent() == null) {
throw new WinRMException("EnumerationContext %s has no content.", context);

Check warning on line 845 in src/main/java/org/metricshub/winrm/service/WinRMService.java

View workflow job for this annotation

GitHub Actions / spotbugs

NP_LOAD_OF_KNOWN_NULL_VALUE

Load of known null value in org.metricshub.winrm.service.WinRMService.getContextIdFrom(EnumerationContextType)
Raw output
 The variable referenced at this point is known to be null due to an earlier check against null. Although this is valid, it might be a mistake (perhaps you intended to refer to a different variable, or perhaps the earlier check to see if the variable is null should have been a check to see if it was non-null).
}

if (context.getContent().isEmpty()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import org.apache.cxf.frontend.ClientProxy;
import org.apache.cxf.jaxws.JaxWsProxyFactoryBean;
import org.apache.cxf.message.Message;
import org.apache.cxf.service.model.EndpointInfo;
import org.apache.cxf.service.model.ServiceInfo;
import org.apache.cxf.transport.http.HTTPConduitFactory;
import org.apache.cxf.transport.http.asyncclient.AsyncHTTPConduit;
Expand Down Expand Up @@ -179,7 +180,7 @@
}

public Client getClient() {
return wsClient;

Check warning on line 183 in src/main/java/org/metricshub/winrm/service/client/WinRMInvocationHandler.java

View workflow job for this annotation

GitHub Actions / spotbugs

EI_EXPOSE_REP

org.metricshub.winrm.service.client.WinRMInvocationHandler.getClient() may expose internal representation by returning WinRMInvocationHandler.wsClient
Raw output
 Returning a reference to a mutable object value stored in one of the object's fields exposes the internal representation of the object. If instances are accessed by untrusted code, and unchecked changes to the mutable object would compromise security or other important properties, you will need to do something different. Returning a new copy of the object is better approach in many situations.
}

@Override
Expand Down Expand Up @@ -403,10 +404,18 @@
client.getOutInterceptors().add(new SignAndEncryptOutInterceptor());

// this is different to endpoint properties
client
.getEndpoint()
.getEndpointInfo()
.setProperty(HTTPConduitFactory.class.getName(), new AsyncHttpEncryptionAwareConduitFactory());
// Register the conduit factory only once: on authentication retries this method is re-invoked on the
// same client, whose cached conduit keeps using the factory it was created with. Replacing the property
// would orphan factory instances, and shutting down the in-use factory would silently downgrade the
// conduit to the synchronous transport (AsyncHTTPConduit.setupConnection checks factory.isShutdown()).
// Reusing the factory also guarantees WinRMService.close() shuts down the instance that owns the
// background threads.
final EndpointInfo endpointInfo = client.getEndpoint().getEndpointInfo();
if (
!(endpointInfo.getProperty(HTTPConduitFactory.class.getName()) instanceof AsyncHttpEncryptionAwareConduitFactory)
) {
endpointInfo.setProperty(HTTPConduitFactory.class.getName(), new AsyncHttpEncryptionAwareConduitFactory());
}

final ServiceInfo serviceInfo = client.getEndpoint().getEndpointInfo().getService();
serviceInfo.setProperty("soap.force.doclit.bare", true);
Expand Down Expand Up @@ -541,12 +550,12 @@
return false;
}
final CredentialsMapKey other = (CredentialsMapKey) obj;
return (
authentication == other.authentication &&
Objects.equals(canonizedRawUsername, other.canonizedRawUsername) &&
Arrays.equals(password, other.password) &&
Objects.equals(ticketCache, other.ticketCache)
);

Check notice on line 558 in src/main/java/org/metricshub/winrm/service/client/WinRMInvocationHandler.java

View workflow job for this annotation

GitHub Actions / PMD

Code Style UselessParentheses

Useless parentheses.
}
}
}
77 changes: 77 additions & 0 deletions src/test/java/org/metricshub/winrm/service/WinRMServiceTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@
import java.nio.file.Paths;
import java.util.List;
import org.apache.cxf.Bus;
import org.apache.cxf.endpoint.Client;
import org.apache.cxf.endpoint.Endpoint;
import org.apache.cxf.service.model.EndpointInfo;
import org.apache.cxf.transport.http.HTTPConduitFactory;
import org.apache.cxf.transport.http.asyncclient.AsyncHTTPConduitFactory;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -268,4 +273,76 @@ void testExecuteWql() throws Exception {
assertEquals(emptyList(), winRMService.executeWql(wqlQuery, timeout));
}
}

@Test
void testCloseShutdownsConduitFactories() throws Exception {
// Use a unique endpoint to avoid interference with other test stubs
final WinRMEndpoint endpointForFactoryTest = new WinRMEndpoint(
null,
"factory-test-host",
null,
"user",
"pwd".toCharArray(),
null
);

// Set up a mock factory that will be stored in the client's endpoint info
final AsyncHTTPConduitFactory mockFactory = mock(AsyncHTTPConduitFactory.class);

// Set up mock endpoint info containing the factory
final EndpointInfo mockEndpointInfo = mock(EndpointInfo.class);
doReturn(mockFactory).when(mockEndpointInfo).getProperty(HTTPConduitFactory.class.getName());

// Set up mock endpoint
final Endpoint mockEndpoint = mock(Endpoint.class);
doReturn(mockEndpointInfo).when(mockEndpoint).getEndpointInfo();

// Set up mock client
final Client mockClient = mock(Client.class);
doReturn(mockEndpoint).when(mockClient).getEndpoint();

// Set up mock invocation handlers that expose the configured client
final WinRMInvocationHandler cmdHandler = mock(WinRMInvocationHandler.class);
doReturn(mockClient).when(cmdHandler).getClient();

final WinRMInvocationHandler wqlHandler = mock(WinRMInvocationHandler.class);
doReturn(mockClient).when(wqlHandler).getClient();

// Override the default stubs for this specific endpoint (registered later, so they take precedence)
MOCKED_WIN_RM_SERVICE
.when(() ->
WinRMService.createWinRMInvocationHandlerInstance(
eq(endpointForFactoryTest),
any(Bus.class),
anyLong(),
isNull(),
isNull(),
anyList()
)
)
.thenReturn(cmdHandler);

MOCKED_WIN_RM_SERVICE
.when(() ->
WinRMService.createWinRMInvocationHandlerInstance(
eq(endpointForFactoryTest),
any(Bus.class),
anyLong(),
anyString(),
isNull(),
anyList()
)
)
.thenReturn(wqlHandler);

// Create the service and immediately close it
// (null ticketCache and authentications: the class-level createInstance stub calling the real
// method only matches isNull() for both)
final WinRMService winRMService = createInstance(endpointForFactoryTest, 30000L, null, null);
assertNotNull(winRMService);
winRMService.close();

// Verify that shutdown() was called on the factory for both the cmd and wql clients
verify(mockFactory, times(2)).shutdown();
}
}
Loading