AgNO3 / jcifs-ng

A cleaned-up and improved version of the jCIFS library
GNU Lesser General Public License v2.1
318 stars 103 forks source link

Chained Responses not (fully) supported #331

Closed ucs-helmut closed 1 year ago

ucs-helmut commented 1 year ago

SMB2 protocol basically supports Chained Responses, similar to Chained Requests. In this case multiple SMB2 blocks are included in a "Transmission Frame". E. g. SMB Implementation on AS400 put the two blocks "Create Response File" and "Close Response" into a single frame.

Both blocks are basically available in Java model and the second block can be retrieved from the object representing the first block using jcifs.internal.CommonServerMessageBlockResponse.getNextResponse() , so seems that the block are correctly mapped.

Anyway, what at least doesn't work properly is managing the Granted Credits. In the Finally-Block of the method jcifs.smb.SmbTransportImpl.sendrecv(CommonServerMessageBlockRequest, T, Set<RequestParam>) just the first SMB Block is considered, but the Granted Credits should be included in the "Close Response" which is the second block. For this reason you run out of Credits after 512 tries / file processing.

This part can be easily fixed by iterate over the response blocks and i already did it on my private code. Anyway, I don't know if additional response blocks are needed to be considered also elsewhere in the SMB implementation.

See an example of what we received from AS400 within one frame: smbChainedResponse.log

mbechler commented 1 year ago

The finally block in https://github.com/AgNO3/jcifs-ng/blob/fbc4b7ad48ad451d871766600bd2edffc3a62af9/src/main/java/jcifs/smb/SmbTransportImpl.java#L1032 should in fact loop over the sent requests and their responses and in my testing works as expected. So I believe this must be a more subtle issue. From the packet capture this looks like a pretty standard chained response, which jcifs-ng uses a lot. Unfortunately that branch does not have suitable logging, can you try debugging this further and/or provide more information (e.g. can you point specific actions that lead to the issue)?

mbechler commented 1 year ago

Correction, a bit too hasty: the trace in fact shows that only the close response grants a credit and the create does not. If this consistently happens, running out of credits is in fact expected, as two credits/sequence numbers are consumed by the corresponding requests. If we don't loose credits somewhere else, this looks like a server bug to me.

ucs-helmut commented 1 year ago

As far as i can see, the while-loop mbechler is referring in the previous reply loops over chained requests and not over chained responses. in my opinion the loop should look like this:

================== start code sample

while ( curReq != null ) {
    if ( curReq.isResponseAsync() ) {
        log.trace("Async");
        break;
    }
    CommonServerMessageBlockResponse resp = curReq.getResponse();

    if ( resp == null ) {
        log.warn("Response not properly set up for" + curReq );
    } else {
        while(resp != null) {
            if ( resp.isReceived() ) {
                grantedCredits += resp.getGrantedCredits();
            }
            resp = resp.getNextResponse();
        }
    }
    CommonServerMessageBlockRequest next = curReq.getNext();
    if ( next == null ) {
        break;
    }
    curReq = next;
}
================ end code sample

Here an explicit loop over the response-chain is added.

I temporarily added this to my local installation and it worked fine so far. Debugging it shows that i processed the second SMB Block. Unfortunately i don't know if managing the Grants is the only thing that has to be considered or additional actions need to be taken.

Furhtermore, mbechler is right, the server application is buggy. It doesn't consider the Credits Granted, neither in the Create Response nor in the Close Response. There is an open incident on IBM to fix this, actually we did this long before i opened the defect for JCifsNG. Anyway, during investigations i came to the point that, even IBM made it correctly, it would not work. To open the defect here i changed the trace for Close Response to what i expected from the Server but forgot to do the same for Create response. So this is my fault, sorry! Anyway, i'll add this information to the IBM Case as well.

mbechler commented 1 year ago

Looping through the requests and looking at the respective responses should yield the same result as looping through the responses, unless there is something wrong with the request/response setup. requests and responses should form two parallel chains. Therefore it looks to me your code will visit responses multiple times and therefore count granted credits multiple times (should be like request1 -> response1 response2, request2 -> response2), which likely fixes your issue of not enough credits being handed out, but will fail horribly if a server actually enforces proper credit usage.

While technically, per spec, a server could send compound responses to non-compound requests, I don't imagine this is the case here (also I don't think that behaviour would make make much sense from a practical standpoint) - and I'm pretty sure that such responses would already fail to parse in the current codebase.

The other way around, if the server responds with individual responses to a compund request, should be properly handled and the sendrecv routine wait until all the individual responses in the chain are received.

Not a fix, but btw. you might be able to mitigate the issue by disallowing jcifs-ng to make compounded requests (either setting jcifs.smb.client.disallowCompound but that may be inconvenient as you will need to add all requests types, or customizing the configuration by overwriting isAllowCompound).

ucs-helmut commented 1 year ago

Many thanks to mbechler for the explanation, sounds very reasonable and i even was able to check in debug session.

Many thanks additionally for describing the workaround. As the AS400 SMB implementation basically supports Chained Requests, for my situation it was enough to set

jcifs.smb.client.disallowCompound=Smb2CreateRequest,Smb2CloseRequest

I'll take care for similar issues and will add other request types if needed. Taking a look at the disallowCompound implementation, it might be helpful to have something like "disallowCompound=all" - but of course can be managed without this option as well.