From 7e7f73766dd9fdc099571deaa8dbe1266c697497 Mon Sep 17 00:00:00 2001 From: Casey Bodley Date: Wed, 27 Oct 2010 14:31:06 -0400 Subject: [PATCH] readdir cookie is stored with nfs41_open_state fixes a memory leak that occurs when a readdir loop doesn't complete, since the cookie was only freed on the last readdir upcall. by storing the cookie with nfs41_open_state, we can avoid passing the cookie to the driver and back, and not worry about having to free it separately Signed-off-by: Casey Bodley --- daemon/nfs41.h | 1 + daemon/nfs41_ops.h | 5 ---- daemon/nfs41_types.h | 5 ++++ daemon/readdir.c | 60 ++++++++++++++++++-------------------------- daemon/upcall.h | 1 - sys/nfs41_driver.c | 18 +++---------- 6 files changed, 33 insertions(+), 57 deletions(-) diff --git a/daemon/nfs41.h b/daemon/nfs41.h index 004505e..3bbcb6f 100644 --- a/daemon/nfs41.h +++ b/daemon/nfs41.h @@ -76,6 +76,7 @@ typedef struct __nfs41_open_state { nfs41_abs_path path; nfs41_path_fh parent; nfs41_path_fh file; + nfs41_readdir_cookie cookie; struct __nfs41_session *session; uint32_t type; bool_t do_close; diff --git a/daemon/nfs41_ops.h b/daemon/nfs41_ops.h index 0eec2d1..10b3a97 100644 --- a/daemon/nfs41_ops.h +++ b/daemon/nfs41_ops.h @@ -643,11 +643,6 @@ typedef struct __nfs41_read_res { /* OP_READDIR */ -typedef struct __nfs41_readdir_cookie { - uint64_t cookie; - unsigned char verf[NFS4_VERIFIER_SIZE]; -} nfs41_readdir_cookie; - typedef struct __nfs41_readdir_args { nfs41_readdir_cookie cookie; uint32_t dircount; diff --git a/daemon/nfs41_types.h b/daemon/nfs41_types.h index 8684722..73065a0 100644 --- a/daemon/nfs41_types.h +++ b/daemon/nfs41_types.h @@ -69,6 +69,11 @@ typedef struct __nfs41_fsid { uint64_t minor; } nfs41_fsid; +typedef struct __nfs41_readdir_cookie { + uint64_t cookie; + unsigned char verf[NFS4_VERIFIER_SIZE]; +} nfs41_readdir_cookie; + typedef struct __netaddr4 { char netid[NFS41_NETWORK_ID_LEN+1]; char uaddr[NFS41_UNIVERSAL_ADDR_LEN+1]; diff --git a/daemon/readdir.c b/daemon/readdir.c index d1e5644..ed892f6 100644 --- a/daemon/readdir.c +++ b/daemon/readdir.c @@ -64,18 +64,13 @@ int parse_readdir(unsigned char *buffer, uint32_t length, nfs41_upcall *upcall) if (status) goto out; status = safe_read(&buffer, &length, &args->state, sizeof(args->state)); if (status) goto out; - status = safe_read(&buffer, &length, &args->cookie, sizeof(args->cookie)); - if (status) goto out; - if (args->cookie == INVALID_HANDLE_VALUE) { - dprintf(1, "upcall passing empty cookie\n"); - args->cookie = NULL; - } + dprintf(1, "parsing NFS41_DIR_QUERY: info_class=%d buf_len=%d " "filter='%s'\n\tInitial\\Restart\\Single %d\\%d\\%d " - "root=0x%p state=0x%p cookie=0x%p\n", + "root=0x%p state=0x%p\n", args->query_class, args->buf_len, args->filter, args->initial, args->restart, args->single, - args->root, args->state, args->cookie); + args->root, args->state); out: return status; } @@ -391,7 +386,7 @@ int readdir_add_dots( *len_out = 0; *last_offset = NULL; - switch (args->cookie->cookie) { + switch (state->cookie.cookie) { case 0: if (entry_buf_len < entry_len + 2) { status = ERROR_BUFFER_OVERFLOW; @@ -451,9 +446,9 @@ int readdir_add_dots( *last_offset = &entry->next_entry_offset; break; } - if (args->cookie->cookie == COOKIE_DOTDOT || - args->cookie->cookie == COOKIE_DOT) - ZeroMemory(args->cookie, sizeof(nfs41_readdir_cookie)); + if (state->cookie.cookie == COOKIE_DOTDOT || + state->cookie.cookie == COOKIE_DOT) + ZeroMemory(&state->cookie, sizeof(nfs41_readdir_cookie)); out: return status; } @@ -474,26 +469,22 @@ int handle_readdir(nfs41_upcall *upcall) args->buf = NULL; args->query_reply_len = 0; - if (args->cookie) { /* cookie exists */ + if (state->cookie.cookie) { /* cookie exists */ if (args->restart) { - dprintf(1, "restarting; clearing previous cookie (%d %p)\n", - args->cookie->cookie, args->cookie); - ZeroMemory(args->cookie, sizeof(nfs41_readdir_cookie)); + dprintf(1, "restarting; clearing previous cookie %llu\n", + state->cookie.cookie); + ZeroMemory(&state->cookie, sizeof(nfs41_readdir_cookie)); } else if (args->initial) { /* shouldn't happen */ - dprintf(1, "*** initial; clearing previous cookie (%d %p)!\n", - args->cookie->cookie, args->cookie); - ZeroMemory(args->cookie, sizeof(nfs41_readdir_cookie)); + dprintf(1, "*** initial; clearing previous cookie %llu!\n", + state->cookie.cookie); + ZeroMemory(&state->cookie, sizeof(nfs41_readdir_cookie)); } else - dprintf(1, "resuming enumeration with cookie %d.\n", - args->cookie->cookie); + dprintf(1, "resuming enumeration with cookie %llu.\n", + state->cookie.cookie); } else { /* cookie is null */ if (args->initial || args->restart) { - dprintf(1, "allocating memory for the 1st readdir cookie\n"); - args->cookie = calloc(1, sizeof(nfs41_readdir_cookie)); - if (args->cookie == NULL) { - status = GetLastError(); - goto out; - } + dprintf(1, "initializing the 1st readdir cookie\n"); + ZeroMemory(&state->cookie, sizeof(nfs41_readdir_cookie)); } else { dprintf(1, "handle_nfs41_readdir: EOF\n"); status = ERROR_NO_MORE_FILES; @@ -532,10 +523,10 @@ fetch_entries: entry_buf_len = 0; eof = 0; } else { - dprintf(2, "calling nfs41_readdir with cookie %d %p \n", - args->cookie->cookie, args->cookie); + dprintf(2, "calling nfs41_readdir with cookie %llu\n", + state->cookie.cookie); status = nfs41_readdir(state->session, &state->file, - &attr_request, args->cookie, entry_buf + dots_len, + &attr_request, &state->cookie, entry_buf + dots_len, &entry_buf_len, &eof); if (status) { dprintf(1, "nfs41_readdir failed with %s\n", @@ -602,7 +593,7 @@ fetch_entries: last_offset = offset; status = NO_ERROR; } - args->cookie->cookie = entry->cookie; + state->cookie.cookie = entry->cookie; /* last entry we got from the server */ if (!entry->next_entry_offset) @@ -628,7 +619,7 @@ fetch_entries: dprintf(1, "we don't need to save a cookie\n"); goto out_free_cookie; } else - dprintf(1, "saving cookie %d %p\n", args->cookie->cookie, args->cookie); + dprintf(1, "saving cookie %llu\n", state->cookie.cookie); out_free_entry: free(entry_buf); @@ -654,8 +645,7 @@ out: } return status; out_free_cookie: - free(args->cookie); - args->cookie = NULL; + state->cookie.cookie = 0; goto out_free_entry; } @@ -667,8 +657,6 @@ int marshall_readdir(unsigned char *buffer, uint32_t *length, nfs41_upcall *upca status = safe_write(&buffer, length, &args->query_reply_len, sizeof(args->query_reply_len)); if (status) goto out; status = safe_write(&buffer, length, args->buf, args->query_reply_len); - if (status) goto out; - status = safe_write(&buffer, length, &args->cookie, sizeof(args->cookie)); out: free(args->buf); return status; diff --git a/daemon/upcall.h b/daemon/upcall.h index fe47df5..e2c0baa 100644 --- a/daemon/upcall.h +++ b/daemon/upcall.h @@ -123,7 +123,6 @@ typedef struct __setexattr_upcall_args { typedef struct __readdir_upcall_args { const char *filter; - nfs41_readdir_cookie *cookie; nfs41_root *root; nfs41_open_state *state; unsigned char *buf; diff --git a/sys/nfs41_driver.c b/sys/nfs41_driver.c index 26263f9..17e2a27 100644 --- a/sys/nfs41_driver.c +++ b/sys/nfs41_driver.c @@ -173,7 +173,6 @@ typedef struct _updowncall_entry { BOOLEAN renamed; } Close; struct { - HANDLE readdir_cookie; HANDLE open_state; HANDLE session; PUNICODE_STRING filter; @@ -330,7 +329,6 @@ typedef struct _NFS41_FOBX { NODE_BYTE_SIZE NodeByteSize; HANDLE nfs41_open_state; - HANDLE nfs41_readdir_cookie; } NFS41_FOBX, *PNFS41_FOBX; #define NFS41GetFileObjectExtension(pFobx) \ (((pFobx) == NULL) ? NULL : (PNFS41_FOBX)((pFobx)->Context)) @@ -812,7 +810,7 @@ NTSTATUS marshal_nfs41_dirquery(nfs41_updowncall_entry *entry, header_len = *len + 2 * sizeof(ULONG) + length_as_ansi(entry->u.QueryFile.filter) + - 3 * sizeof(BOOLEAN) + 3 * sizeof(HANDLE); + 3 * sizeof(BOOLEAN) + 2 * sizeof(HANDLE); if (header_len > buf_len) { status = STATUS_INSUFFICIENT_RESOURCES; goto out; @@ -833,17 +831,15 @@ NTSTATUS marshal_nfs41_dirquery(nfs41_updowncall_entry *entry, RtlCopyMemory(tmp, &entry->u.QueryFile.session, sizeof(HANDLE)); tmp += sizeof(HANDLE); RtlCopyMemory(tmp, &entry->u.QueryFile.open_state, sizeof(HANDLE)); - tmp += sizeof(HANDLE); - RtlCopyMemory(tmp, &entry->u.QueryFile.readdir_cookie, sizeof(HANDLE)); *len = header_len; DbgP("filter='%wZ' class=%d\n\t1st\\restart\\single=%d\\%d\\%d " - "session=0x%x open_state=0x%x readdir_cookie=0x%x\n", + "session=0x%x open_state=0x%x\n", entry->u.QueryFile.filter, entry->u.QueryFile.InfoClass, entry->u.QueryFile.initial_query, entry->u.QueryFile.restart_scan, entry->u.QueryFile.return_single, entry->u.QueryFile.session, - entry->u.QueryFile.open_state, entry->u.QueryFile.readdir_cookie); + entry->u.QueryFile.open_state); out: DbgEx(); return status; @@ -1424,10 +1420,6 @@ nfs41_downcall ( } cur->u.QueryFile.buf_len = tmp->u.QueryFile.buf_len; RtlCopyMemory(cur->u.QueryFile.buf, buf, tmp->u.QueryFile.buf_len); - if (tmp->opcode == NFS41_DIR_QUERY) { - buf += tmp->u.QueryFile.buf_len; - RtlCopyMemory(&cur->u.QueryFile.readdir_cookie, buf, sizeof(HANDLE)); - } break; case NFS41_SYMLINK: if (cur->u.Symlink.set) @@ -2705,7 +2697,6 @@ NTSTATUS nfs41_Create( print_fobx(1, RxContext->pFobx); nfs41_fobx = (PNFS41_FOBX)(RxContext->pFobx)->Context; nfs41_fobx->nfs41_open_state = entry->u.Open.open_state; - nfs41_fobx->nfs41_readdir_cookie = INVALID_HANDLE_VALUE; // we get attributes only for data access and file (not directories) if (Fcb->OpenCount > 0) @@ -3074,7 +3065,6 @@ NTSTATUS nfs41_QueryDirectory ( if (status) goto out; entry->u.QueryFile.open_state = nfs41_fobx->nfs41_open_state; - entry->u.QueryFile.readdir_cookie = nfs41_fobx->nfs41_readdir_cookie; entry->u.QueryFile.InfoClass = InfoClass; entry->u.QueryFile.buf_len = RxContext->Info.LengthRemaining; entry->u.QueryFile.buf = RxContext->Info.Buffer; @@ -3089,14 +3079,12 @@ NTSTATUS nfs41_QueryDirectory ( goto out; } - nfs41_fobx->nfs41_readdir_cookie = INVALID_HANDLE_VALUE; if (entry->status == STATUS_BUFFER_TOO_SMALL) { DbgP("ERROR: buffer too small provided %d need %d\n", RxContext->Info.LengthRemaining, entry->u.QueryFile.buf_len); RxContext->InformationToReturn = entry->u.QueryFile.buf_len; status = STATUS_BUFFER_TOO_SMALL; } else if (entry->status == STATUS_SUCCESS) { - nfs41_fobx->nfs41_readdir_cookie = entry->u.QueryFile.readdir_cookie; RtlCopyMemory(RxContext->Info.Buffer, entry->u.QueryFile.buf, entry->u.QueryFile.buf_len); RxContext->Info.LengthRemaining -= entry->u.QueryFile.buf_len;