Home
My Posts

Archive
Login
Signup

Recent Posts

Free subdomains

Want your own xyz.kpaste.net sub-domain for your community? Just type the address into your browser address bar (foo.xyz.kpaste.net is also supported).

Suggestions/Bugs?

Email us at . Try to include as much info as possible.

About

Pastebin is a tool for collaborative debugging or editing.

 

msnfs41client: Patches for fixing SRWLock/CRITSEC waiting before |free()|, getattr debug and misc, 2024-02-28 ...
Posted by Anonymous on Wed 28th Feb 2024 16:31
raw | new post

Submit a correction or amendment below (click here to make a fresh posting)
After submitting an amendment, you'll be able to view the differences between the old and new posts easily.

Syntax highlighting: None Bash C C++ CSS Diff HTML mIRC Script Perl PHP Python Smarty XML ---------------------------- ABAP Actionscript ADA Apache Log AppleScript APT sources.list ASM ASP AutoIT Bash C C# C++ C++ (with QT) CSS Diff HTML INI (Config Files) Java Javascript Lua Make mIRC Script MySQL NSIS Perl PHP Python Q(uick)BASIC robots Ruby Ruby on Rails SDLBasic Smarty SQL TCL Text VB.NET Visual BASIC Visual Fox Pro Visual Prolog Windows Registry Files XML Xorg.conf

To highlight particular lines, prefix each line with {%HIGHLIGHT}
From 418713079308fd6d9a041cc8d25d54bf3a472a8e Mon Sep 17 00:00:00 2001 From: Roland Mainz <roland.mainz@nrubsig.org> Date: Wed, 28 Feb 2024 10:14:20 +0100 Subject: [PATCH 1/4] daemon: Add missing init of |state->lock| in |create_open_state()| Add missing |InitializeSRWLock(&state->lock);| in |create_open_state()|, and add a comment who releases the initial |ref_count|. Signed-off-by: Cedric Blancher <cedric.blancher@gmail.com> --- daemon/open.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/daemon/open.c b/daemon/open.c index f605f1e..0c502db 100644 --- a/daemon/open.c +++ b/daemon/open.c @@ -3,6 +3,7 @@ * * Olga Kornievskaia <aglo@umich.edu> * Casey Bodley <cbodley@umich.edu> + * Roland Mainz <roland.mainz@nrubsig.org> * * This library is free software; you can redistribute it and/or modify it * under the terms of the GNU Lesser General Public License as published by @@ -61,7 +62,8 @@ static int create_open_state( StringCchPrintfA((LPSTR)state->owner.owner, NFS4_OPAQUE_LIMIT, "%u", open_owner_id); state->owner.owner_len = (uint32_t)strlen((const char*)state->owner.owner); - state->ref_count = 1; + InitializeSRWLock(&state->lock); + state->ref_count = 1; /* will be released in |cleanup_close()| */ list_init(&state->locks.list); list_init(&state->client_entry); InitializeCriticalSection(&state->locks.lock); -- 2.43.0 From 936afc151ba8ad90553bcd3a96818c913e5b9db8 Mon Sep 17 00:00:00 2001 From: Roland Mainz <roland.mainz@nrubsig.org> Date: Wed, 28 Feb 2024 16:16:38 +0100 Subject: [PATCH 2/4] daemon: Make sure noone uses SRWLocks and critical setions before |free()| Make sure noone uses SRWLocks and critical setions before using |free()| on the underlying memory. Signed-off-by: Cedric Blancher <cedric.blancher@gmail.com> --- daemon/getattr.c | 4 ++-- daemon/namespace.c | 2 ++ daemon/nfs41_client.c | 5 +++++ daemon/nfs41_server.c | 3 +++ daemon/open.c | 16 +++++++++++++++ daemon/util.c | 47 +++++++++++++++++++++++++++++++++++++++++++ daemon/util.h | 3 +++ 7 files changed, 78 insertions(+), 2 deletions(-) diff --git a/daemon/getattr.c b/daemon/getattr.c index 9e07df9..a3727aa 100644 --- a/daemon/getattr.c +++ b/daemon/getattr.c @@ -80,11 +80,11 @@ static int parse_getattr(unsigned char *buffer, uint32_t length, nfs41_upcall *u */ volatile int ok = 0; __try { - if (upcall->state_ref->session->client->server != NULL) + if (upcall->state_ref->session->client != NULL) ok++; } __except(EXCEPTION_EXECUTE_HANDLER) { - eprintf("parse_getattr: Exception accessing upcall->state_ref->session->client->server\n"); + eprintf("parse_getattr: Exception accessing upcall->state_ref->session->client\n"); } if (ok != 1) { status = ERROR_INVALID_PARAMETER; diff --git a/daemon/namespace.c b/daemon/namespace.c index 87425a0..1d99e98 100644 --- a/daemon/namespace.c +++ b/daemon/namespace.c @@ -81,6 +81,8 @@ static void root_free( DPRINTF(NSLVL, ("--> nfs41_root_free()\n")); + EASSERT(waitcriticalsection(&root->lock) == TRUE); + /* free clients */ list_for_each_tmp(entry, tmp, &root->clients) nfs41_client_free(client_entry(entry)); diff --git a/daemon/nfs41_client.c b/daemon/nfs41_client.c index 1d043e0..7de1d56 100644 --- a/daemon/nfs41_client.c +++ b/daemon/nfs41_client.c @@ -207,6 +207,11 @@ void nfs41_client_free( IN nfs41_client *client) { DPRINTF(2, ("nfs41_client_free(%llu)\n", client->clnt_id)); + + EASSERT(waitcriticalsection(&client->recovery.lock) == TRUE); + EASSERT(waitSRWlock(&client->exid_lock) == TRUE); + EASSERT(waitcriticalsection(&client->state.lock) == TRUE); + nfs41_client_delegation_free(client); if (client->session) nfs41_session_free(client->session); nfs41_destroy_clientid(client->rpc, client->clnt_id); diff --git a/daemon/nfs41_server.c b/daemon/nfs41_server.c index 9902954..8192fed 100644 --- a/daemon/nfs41_server.c +++ b/daemon/nfs41_server.c @@ -129,6 +129,9 @@ static void server_free( IN nfs41_server *server) { DPRINTF(SRVLVL, ("server_free('%s')\n", server->owner)); + + EASSERT(waitSRWlock(&server->addrs.lock) == TRUE); + nfs41_superblock_list_free(&server->superblocks); nfs41_name_cache_free(&server->name_cache); free(server); diff --git a/daemon/open.c b/daemon/open.c index 0c502db..0732aa2 100644 --- a/daemon/open.c +++ b/daemon/open.c @@ -85,6 +85,12 @@ static void open_state_free( IN nfs41_open_state *state) { struct list_entry *entry, *tmp; + + EASSERT(waitcriticalsection(&state->ea.lock) == TRUE); + EASSERT(waitcriticalsection(&state->locks.lock) == TRUE); + + EASSERT(waitSRWlock(&state->lock) == TRUE); + EASSERT(waitSRWlock(&state->path.lock) == TRUE); /* free associated lock state */ list_for_each_tmp(entry, tmp, &state->locks.list) @@ -93,6 +99,12 @@ static void open_state_free( nfs41_delegation_deref(state->delegation.state); if (state->ea.list != INVALID_HANDLE_VALUE) free(state->ea.list); + + DeleteCriticalSection(&state->ea.lock); + DeleteCriticalSection(&state->locks.lock); + + EASSERT(state->ref_count == 0); + free(state); } @@ -119,6 +131,8 @@ void nfs41_open_state_ref( return; #endif /* NFS41_DRIVER_STABILITY_HACKS */ + EASSERT(state->ref_count > 0); + const LONG count = InterlockedIncrement(&state->ref_count); DPRINTF(2, ("nfs41_open_state_ref('%s') count %d\n", state->path.path, count)); @@ -127,6 +141,8 @@ void nfs41_open_state_ref( void nfs41_open_state_deref( IN nfs41_open_state *state) { + EASSERT(state->ref_count > 0); + const LONG count = InterlockedDecrement(&state->ref_count); DPRINTF(2, ("nfs41_open_state_deref('%s') count %d\n", state->path.path, count)); diff --git a/daemon/util.c b/daemon/util.c index 4c0fd4c..e2774d0 100644 --- a/daemon/util.c +++ b/daemon/util.c @@ -581,3 +581,50 @@ BOOL subcmd_readcmdoutput(subcmd_popen_context *pinfo, char *buff, size_t buff_s { return ReadFile(pinfo->hReadPipe, buff, (DWORD)buff_size, num_buff_read_ptr, NULL); } + +/* + * |waitSRWlock()| - Wait for outstanding locks (usually used + * before disposing (e.g. |free()| the memory of the structure + * of the lock) a SRW lock. + * Returns |TRUE| if we didn't had to wait for another thread + * to release the lock first. + */ +bool_t waitSRWlock(PSRWLOCK srwlock) +{ + bool_t srw_locked; + + /* Check whether something is still using the lock */ + srw_locked = TryAcquireSRWLockExclusive(srwlock); + if (srw_locked) { + ReleaseSRWLockExclusive(srwlock); + } + else { + AcquireSRWLockExclusive(srwlock); + ReleaseSRWLockExclusive(srwlock); + } + return srw_locked; +} + +/* + * |waitcriticalsection()| - Wait for other threads using the + * CRITICAL_SECTION (usually used before disposing (e.g. + * |free()| the memory of the structure of the CRITICAL_SECTION) + * a CRITICAL_SECTION. + * Returns |TRUE| if we didn't had to wait for another thread + * to release the lock first. + */ +bool_t waitcriticalsection(LPCRITICAL_SECTION cs) +{ + bool_t cs_locked; + + /* Check whether something is still using the critical section */ + cs_locked = TryEnterCriticalSection(cs); + if (cs_locked) { + LeaveCriticalSection(cs); + } + else { + EnterCriticalSection(cs); + LeaveCriticalSection(cs); + } + return cs_locked; +} diff --git a/daemon/util.h b/daemon/util.h index 9e1c3e5..140a3bd 100644 --- a/daemon/util.h +++ b/daemon/util.h @@ -266,4 +266,7 @@ subcmd_popen_context *subcmd_popen(const char *command); int subcmd_pclose(subcmd_popen_context *pinfo); BOOL subcmd_readcmdoutput(subcmd_popen_context *pinfo, char *buff, size_t buff_size, DWORD *num_buff_read_ptr); +bool_t waitSRWlock(PSRWLOCK srwlock); +bool_t waitcriticalsection(LPCRITICAL_SECTION cs); + #endif /* !__NFS41_DAEMON_UTIL_H__ */ -- 2.43.0 From 1031499a998579bf180cece8ba3fdc6e40de4de1 Mon Sep 17 00:00:00 2001 From: Roland Mainz <roland.mainz@nrubsig.org> Date: Wed, 28 Feb 2024 16:34:04 +0100 Subject: [PATCH 3/4] daemon: |parse_getattr()| should report address of |state_ref| on exception |parse_getattr()| should report address of |upcall->state_ref| on exception. Signed-off-by: Cedric Blancher <cedric.blancher@gmail.com> --- daemon/getattr.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/daemon/getattr.c b/daemon/getattr.c index a3727aa..3c69db3 100644 --- a/daemon/getattr.c +++ b/daemon/getattr.c @@ -84,7 +84,9 @@ static int parse_getattr(unsigned char *buffer, uint32_t length, nfs41_upcall *u ok++; } __except(EXCEPTION_EXECUTE_HANDLER) { - eprintf("parse_getattr: Exception accessing upcall->state_ref->session->client\n"); + eprintf("parse_getattr: Exception accessing " + "upcall->state_ref(=0x%p)->session->client\n", + upcall->state_ref); } if (ok != 1) { status = ERROR_INVALID_PARAMETER; -- 2.43.0 From b2ffbe8fdea9b0fcf6ea9e91b218b577b9ae2893 Mon Sep 17 00:00:00 2001 From: Roland Mainz <roland.mainz@nrubsig.org> Date: Wed, 28 Feb 2024 16:36:46 +0100 Subject: [PATCH 4/4] tests: manual testing: libisl-devel is now required for gcc build manual_testing.txt Document that Cygwin 3.6.x requires package "libisl-devel" to build gcc. Signed-off-by: Cedric Blancher <cedric.blancher@gmail.com> --- tests/manual_testing.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/manual_testing.txt b/tests/manual_testing.txt index a0c9408..c9c34a6 100644 --- a/tests/manual_testing.txt +++ b/tests/manual_testing.txt @@ -19,6 +19,7 @@ # libmpfr-devel # libmpc-devel # libintl-devel +# libisl-devel # flex # bison # ---- snip ---- -- 2.43.0

Title:

Privacy: Public Private

How long should your post be retained?

15 mins an hour a day a week a month forever

All content is user-submitted.
The administrators of this site (kpaste.net) are not responsible for their content.
Abuse reports should be emailed to us at