Update parser so all paths may contain CGI/query elements

[dcache]

Update parser so all paths may contain CGI/query elements

[alrossi]

Will have to review the protocol to see what other operations include path information (besides the obvious ones). Do we even support things like kXR_putfile and kXR_getfile ? ... I don't remember. I guess I'm the logical one to take care of this. Al

…

________________________________________________ Albert L. Rossi Application Developer & Systems Analyst III Scientific Computing Division, Data Movement Development FCC 229A Mail Station 369 (FCC 2W) Fermi National Accelerator Laboratory Batavia, IL 60510 [phone]

________________________________ From: Paul Millar <[email protected]> Sent: Tuesday, October 30, 2018 4:13 AM To: dCache/xrootd4j Cc: Subscribed Subject: [dCache/xrootd4j] Update parser so all paths may contain CGI/query elements (#23) As clarified in xrootd/xrootd#850<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_xrootd_xrootd_issues_850&d=DwMCaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=60rQ0HHqHmEY1P6VSdyuTQ&m=BxlrlZ45LTKZhzJvA_4MzZica18sWBmvpvRjYz2i6TM&s=SyYWWf2egiv1nNLlSgyYam5QbSFIXsuimKyN36LjQXc&e=>, all paths in any xrootd request may contain CGI elements (a query string). Currently all operations, (except kXR_open and kXR_mv) treat all paths as if they have no CGI elements, as the xrootd protocol spec currently doesn't mention this behaviour. This results in clients potentially sending valid paths to xroot4j that then fail. To fix this, all operations should be examined and any paths should remove the query string argument. Additional getter methods should be added to provide access to these optional elements. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_dCache_xrootd4j_issues_23&d=DwMCaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=60rQ0HHqHmEY1P6VSdyuTQ&m=BxlrlZ45LTKZhzJvA_4MzZica18sWBmvpvRjYz2i6TM&s=-9T5zguO9EA97HEEmf6KlBk3Iaw7Zns4gKXGE2OIR9o&e=>, or mute the thread<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ADzQnDiLv4r-2DZuDf3OdHM-2DP7-5FNnWHzcSks5uqBhVgaJpZM4YBesH&d=DwMCaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=60rQ0HHqHmEY1P6VSdyuTQ&m=BxlrlZ45LTKZhzJvA_4MzZica18sWBmvpvRjYz2i6TM&s=ty30Cx6GJtOHTGhxPpiP9WUx4mKvbPv-25khgmrcWa0&e=>.

[paulmillar]

Yes, that's why I created this issue, we need to go through all operations and identify which ones have a path and make sure we strip off any query-part.

I think we should also take this advantage of this opportunity to add unit tests for this. See StatRequestTest for one possible way of achieving this.

[alrossi]

Let me clarify something here.

First, there are already a set of requests:

DirListRequest
LocateRequest
MkDirRequest
OpenRequest
RmDirRequest
RmRequest
StatRequest

which sublcass PathRequest. When the message buffer is parsed in their constructors, it is already split on a the potential opaque delimiter ('?').

So we are already supporting queries on those requests.

The case of mv seems to be special, because two paths are involved.

The code for the the MvRequest thus is a one-off.

The issue seems to be in the assumption that only the target path can have the opaque (query) string (that is, it will follow the target path, not the source).

public MvRequest(ByteBuf buffer) {
        super(buffer, kXR_mv);

        int dlen = buffer.getInt(20);
        int end = 24 + dlen;

        int psep = buffer.indexOf(24, end, (byte)0x20);
        int osep = buffer.indexOf(psep, end, (byte)0x3f);

        if (psep == -1) {
            throw new IllegalArgumentException("kXR_mv needs two paths!");
        }

        if (osep > -1) {
            sourcePath = buffer.toString(24,
                                          psep - 24,
                                          US_ASCII);
            targetPath = buffer.toString(psep+1,
                                          osep - (psep + 1),
                                          US_ASCII);
            opaque = buffer.toString(osep + 1,
                                      end - (osep + 1),
                                      US_ASCII);
        } else {
            sourcePath = buffer.toString(24,
                                          psep - 24,
                                          US_ASCII);
            targetPath = buffer.toString(psep+1,
                                          end - (psep + 1),
                                          US_ASCII);
            opaque = null;
        }
    }

I don't believe we need, therefore, to fix a general problem, just this class.

[alrossi]

https://rb.dcache.org/r/11422/

needs more testing

I am not sure that xrdfs actually is sending the query part

[alrossi]

Testing complete, patch committed to master, with pull request for 3.3

#27