[NFS-Ganesha-Devel] [M] Change in ...nfs-ganesha[next]: XDR: stop consuming the entry3/entryplus3 discriminator twice

Mark Ruvald Pedersen has uploaded this change for review. ( https://review.gerrithub.io/c/ffilz/nfs-ganesha/+/1246962?usp=email ) Change subject: XDR: stop consuming the entry3/entryplus3 discriminator twice ...................................................................... XDR: stop consuming the entry3/entryplus3 discriminator twice xdr_dirlist3() and xdr_dirlistplus3() wrapped the entry3/entryplus3 list walkers in xdr_pointer(). Both layers read a "value follows" boolean, but RFC 1813 puts only one in front of each element: [bool=TRUE][entry][bool=TRUE][entry] ... [bool=FALSE] xdr_pointer() consumed the real boolean, so the walker then read the high 32 bits of the first entry's fileid3 as its own discriminator. Both outcomes are broken, and which one you get depends on the server: - High word zero: the walker stops immediately, leaving the zeroed node xdr_pointer() had already allocated as an entry whose name is NULL, and eof is then read from the fileid's low word. FSAL_PROXY_V3 walks that entry and calls strcmp() on the NULL name. - High word non-zero: the stream desyncs, so the following filename3 length is read from the wrong offset. Both were observed against NetApp ONTAP volumes. One puts the significant bits in the upper half -- a fileid of 9242200323076915308 is 0x8042e43a0000006c, so XDR puts 0x8042e43a on the wire first -- and READDIRPLUS against it fails with xdr_string_decode ERROR size 785966203 > max 8192 proxyv3_call: Failed to do xdr_replymsg Another volume hands out fileids below 2^32 and crashed instead. This is very likely the bug behind issue #1142. Only FSAL_PROXY_V3 decodes these. The server encodes non-empty replies through the uio path in xdr_dirlist3_encode() and xdr_dirlistplus3_encode(), and so never exercised the defect; empty continuations do reach the fallback encoder, but both the old and the new code emit [FALSE][eof] correctly there. Encode and decode were wrong symmetrically, so a round trip through this code agreed with itself even though neither direction matched the wire format. Give the walkers the whole chain: they now take the address of the caller's head pointer and own the per-element boolean, and the dirlist functions call them directly instead of through xdr_pointer(). They are renamed to xdr_entry3_list() and xdr_entryplus3_list() and made static, rather than keeping the established rpcgen-style xdr_entry3()/xdr_entryplus3() names with a changed signature. Both call sites are in this file, so nothing else needs them, and an out-of-tree caller now fails to link instead of silently reinterpreting a fileid3 as a pointer. A FALSE discriminator also has to clear the caller's head pointer the way xdr_pointer() did, otherwise decoding an empty or shorter list into a reused destination leaves the previous entries in place. While here, fix a use-after-free on the XDR_FREE path. It saved &(*ent)->nextentry and dereferenced it after freeing the object that field lives in; save the successor by value instead. Note that this fixes directory listing only. Reading file contents through FSAL_PROXY_V3 additionally requires the xdrmem_fillbufs() fix in ntirpc, submitted as nfs-ganesha/ntirpc#408; without it a proxied READ returns the front of the RPC reply rather than the payload. This patch deliberately does not bump the src/libntirpc submodule, so it applies cleanly on top of the current pin. Signed-off-by: Mark Ruvald Pedersen <wabsie(a)gmail.com&gt; Change-Id: Ib547cd2223cc07be8c02f7d7a3703ed018b1889d --- M src/Protocols/XDR/xdr_nfs23.c M src/include/nfs23.h 2 files changed, 102 insertions(+), 48 deletions(-) git pull ssh://review.gerrithub.io:29418/ffilz/nfs-ganesha refs/changes/62/1246962/1 -- To view, visit https://review.gerrithub.io/c/ffilz/nfs-ganesha/+/1246962?usp=email To unsubscribe, or for help writing mail filters, visit https://review.gerrithub.io/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: ffilz/nfs-ganesha Gerrit-Branch: next Gerrit-Change-Id: Ib547cd2223cc07be8c02f7d7a3703ed018b1889d Gerrit-Change-Number: 1246962 Gerrit-PatchSet: 1 Gerrit-Owner: Mark Ruvald Pedersen <wabsie(a)gmail.com&gt;