Skip to content

Commit

Permalink
nfs3: fix readdir entry size calculation
Browse files Browse the repository at this point in the history
Motivation:
when estimating readdir reply size we should not forget about boolean
flag that indicates the presence of a next entry.

Modification:
fix entry overhead size.

Result:
The returned xdr doesn't overflow expected reply size.

Acked-by: Paul Millar
Target: master, 0.22
(cherry picked from commit ea6f71a)
Signed-off-by: Tigran Mkrtchyan <[email protected]>
  • Loading branch information
kofemann committed Jan 5, 2021
1 parent f349133 commit 3e379c6
Show file tree
Hide file tree
Showing 5 changed files with 57 additions and 6 deletions.
15 changes: 13 additions & 2 deletions core/src/main/java/org/dcache/nfs/v3/NfsServerV3.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2009 - 2020 Deutsches Elektronen-Synchroton,
* Copyright (c) 2009 - 2021 Deutsches Elektronen-Synchroton,
* Member of the Helmholtz Association, (DESY), HAMBURG, GERMANY
*
* This library is free software; you can redistribute it and/or modify
Expand Down Expand Up @@ -155,7 +155,18 @@
public class NfsServerV3 extends nfs3_protServerStub {

// needed to calculate replay size for READDIR3 and READDIRPLUS3
private static final int ENTRY3_SIZE = 24;
/*
* readdir entry size + overhead
* 8 (cookie) + 8 (file id) + 4 (name len) + 4 (smallest padded name) + 4 (boolean has next)
*/
private static final int ENTRY3_SIZE = 28;

/*
* readdir_plus entry size + overhead, without file handle
* 8 (cookie) + 8 (file id) + 4 (name len) + 4 (smallest padded name) + 4 (boolean has next) +
* 4 (boolean attrs follow, always true) + 84 (fattr3) + 4 (boolean has handle, always true) +
* 4 (handle len)
*/
private static final int ENTRYPLUS3_SIZE = 124;
private static final int READDIR3RESOK_SIZE = 104;
private static final int READDIRPLUS3RESOK_SIZE = 104;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@
import org.junit.Before;
import org.junit.Test;

import static org.dcache.testutils.XdrHelper.calculateSize;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.lessThanOrEqualTo;
import static org.mockito.Mockito.*;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
Expand Down Expand Up @@ -177,7 +181,8 @@ public void testEofIfLastEntryDoesNotFit() throws Exception {
n++;
}

assertEquals("Not all antries returned", dirContents.size() - 1, n);
assertThat("reply overflow", calculateSize(result), is(lessThanOrEqualTo(3480)));
assertEquals("Not all entries returned", dirContents.size() - 1, n);
assertFalse("The last entry is missed", result.resok.reply.eof);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@
import org.junit.Before;
import org.junit.Test;

import static org.dcache.testutils.XdrHelper.calculateSize;
import static org.hamcrest.CoreMatchers.*;
import static org.hamcrest.MatcherAssert.*;
import static org.hamcrest.Matchers.lessThanOrEqualTo;
import static org.mockito.Mockito.*;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
Expand Down Expand Up @@ -165,7 +169,7 @@ public void testEofIfLastEntryDoesNotFit() throws Exception {
when(vfs.list(eq(dirInode), any(), anyLong())).thenReturn(new DirectoryStream(cookieVerifier, dirContents));

RpcCall call = new RpcCallBuilder().from("1.2.3.4", "someHost.acme.com", 42).nfs3().noAuth().build();
READDIR3args args = NfsV3Ops.readDir(dirHandle, 0, cookieVerifier, 770); // only 22 entries will fit
READDIR3args args = NfsV3Ops.readDir(dirHandle, 0, cookieVerifier, 836); // only 22 entries will fit
READDIR3res result = nfsServer.NFSPROC3_READDIR_3(call, args);

int n = 0;
Expand All @@ -176,7 +180,8 @@ public void testEofIfLastEntryDoesNotFit() throws Exception {
n++;
}

assertEquals("Not all antries returned", dirContents.size() - 1, n);
assertThat("reply overflow", calculateSize(result), is(lessThanOrEqualTo(836)));
assertEquals("Not all entries returned", dirContents.size() - 1, n);
assertFalse("The last entry is missed", result.resok.reply.eof);
}
}
11 changes: 10 additions & 1 deletion core/src/test/java/org/dcache/nfs/v4/OperationREADDIRTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,16 @@
import org.junit.Test;
import org.junit.Before;

import static org.hamcrest.MatcherAssert.*;
import static org.dcache.nfs.v4.NfsTestUtils.generateRpcCall;
import static org.junit.Assert.*;
import static org.dcache.testutils.XdrHelper.calculateSize;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.Matchers.lessThanOrEqualTo;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.fail;
import static org.mockito.Mockito.*;

public class OperationREADDIRTest {
Expand Down Expand Up @@ -111,6 +119,7 @@ public void testEofIfLastEntryDoesNotFit() throws Exception {

listed();

assertThat("reply overflow", calculateSize(result), is(lessThanOrEqualTo(1000)));
assertEquals("Not all entries returned", dirEntries.length - 1, entryCount());
assertFalse("The last entry is missed", result.opreaddir.resok4.reply.eof);
}
Expand Down
21 changes: 21 additions & 0 deletions core/src/test/java/org/dcache/testutils/XdrHelper.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package org.dcache.testutils;

import org.dcache.oncrpc4j.xdr.Xdr;
import org.dcache.oncrpc4j.xdr.XdrAble;

public class XdrHelper {
private XdrHelper() {}


public static int calculateSize (XdrAble xdrAble) {
try {
Xdr xdr = new Xdr(128);
xdr.beginEncoding();
xdrAble.xdrEncode(xdr);
xdr.endEncoding();
return xdr.getBytes().length;
} catch (Exception e) {
throw new AssertionError("object does not survive xdr encoding", e);
}
}
}

0 comments on commit 3e379c6

Please sign in to comment.