This means that the diagnostic message is no longer shown when secur32 is loaded.
From: Hans Leidekker hans@codeweavers.com
And return an error if it can't be forked. --- dlls/msv1_0/main.c | 17 ----------------- dlls/msv1_0/unixlib.c | 43 +++++++------------------------------------ dlls/msv1_0/unixlib.h | 1 - 3 files changed, 7 insertions(+), 54 deletions(-)
diff --git a/dlls/msv1_0/main.c b/dlls/msv1_0/main.c index 409906315e9..a8c5dab8b0a 100644 --- a/dlls/msv1_0/main.c +++ b/dlls/msv1_0/main.c @@ -43,11 +43,6 @@ WINE_DEFAULT_DEBUG_CHANNEL(ntlm); static ULONG ntlm_package_id; static LSA_DISPATCH_TABLE lsa_dispatch;
-static NTSTATUS ntlm_check_version(void) -{ - return WINE_UNIX_CALL( unix_check_version, NULL ); -} - static void ntlm_cleanup( struct ntlm_ctx *ctx ) { WINE_UNIX_CALL( unix_cleanup, ctx ); @@ -113,12 +108,6 @@ static NTSTATUS NTAPI ntlm_LsaApInitializePackage( ULONG package_id, LSA_DISPATC TRACE( "%#lx, %p, %s, %s, %p\n", package_id, dispatch, debugstr_as(database), debugstr_as(confidentiality), package_name );
- if (ntlm_check_version()) - { - ERR( "no NTLM support, expect problems\n" ); - return STATUS_UNSUCCESSFUL; - } - if (!(str = dispatch->AllocateLsaHeap( sizeof(*str) + sizeof("NTLM" )))) return STATUS_NO_MEMORY; ptr = (char *)(str + 1); memcpy( ptr, "NTLM", sizeof("NTLM") ); @@ -135,12 +124,6 @@ static NTSTATUS NTAPI ntlm_SpInitialize( ULONG_PTR package_id, SECPKG_PARAMETERS LSA_SECPKG_FUNCTION_TABLE *lsa_function_table ) { TRACE( "%#Ix, %p, %p\n", package_id, params, lsa_function_table ); - - if (ntlm_check_version()) - { - ERR( "no NTLM support, expect problems\n" ); - return STATUS_UNSUCCESSFUL; - } return STATUS_SUCCESS; }
diff --git a/dlls/msv1_0/unixlib.c b/dlls/msv1_0/unixlib.c index 95c92233b37..b385f4a452f 100644 --- a/dlls/msv1_0/unixlib.c +++ b/dlls/msv1_0/unixlib.c @@ -47,7 +47,6 @@ extern char **environ; #endif
-WINE_DEFAULT_DEBUG_CHANNEL(ntlm); WINE_DECLARE_DEBUG_CHANNEL(winediag);
#define INITIAL_BUFFER_SIZE 200 @@ -164,7 +163,8 @@ static NTSTATUS ntlm_fork( void *args ) const struct fork_params *params = args; struct ntlm_ctx *ctx = params->ctx; posix_spawn_file_actions_t file_actions; - int pipe_in[2], pipe_out[2]; + int pipe_in[2], pipe_out[2], err; + NTSTATUS status = STATUS_SUCCESS;
#ifdef HAVE_PIPE2 if (pipe2( pipe_in, O_CLOEXEC ) < 0) @@ -198,10 +198,14 @@ static NTSTATUS ntlm_fork( void *args ) posix_spawn_file_actions_addclose( &file_actions, pipe_in[0] ); posix_spawn_file_actions_addclose( &file_actions, pipe_in[1] );
- if (posix_spawnp( &ctx->pid, params->argv[0], &file_actions, NULL, params->argv, environ )) + if ((err = posix_spawnp( &ctx->pid, params->argv[0], &file_actions, NULL, params->argv, environ ))) { ctx->pid = -1; write( pipe_in[1], "BH\n", 3 ); + ERR_(winediag)( "Can't start ntlm_auth (%s). " + "Usually you can find it in the winbind package of your distribution.\n", + strerror(err) ); + status = STATUS_UNSUCCESSFUL; }
ctx->pipe_in = pipe_in[0]; @@ -211,37 +215,6 @@ static NTSTATUS ntlm_fork( void *args )
posix_spawn_file_actions_destroy( &file_actions );
- return SEC_E_OK; -} - -static NTSTATUS ntlm_check_version( void *args ) -{ - struct ntlm_ctx ctx = { 0 }; - char *argv[3], buf[80]; - NTSTATUS status = STATUS_DLL_NOT_FOUND; - struct fork_params params = { &ctx, argv }; - int len; - - ctx.mode = MODE_CLIENT; - argv[0] = (char *)"ntlm_auth"; - argv[1] = (char *)"--version"; - argv[2] = NULL; - if (ntlm_fork( ¶ms ) != SEC_E_OK) return status; - - if ((len = read( ctx.pipe_in, buf, sizeof(buf) - 1 )) > 8) - { - char *newline; - - if ((newline = memchr( buf, '\n', len ))) *newline = 0; - else buf[len] = 0; - - TRACE( "detected ntlm_auth version %s\n", debugstr_a(buf) ); - status = STATUS_SUCCESS; - } - - if (status) ERR_(winediag)( "ntlm_auth was not found. Make sure that ntlm_auth >= 3.0.25 is in your path. " - "Usually, you can find it in the winbind package of your distribution.\n" ); - ntlm_cleanup( &ctx ); return status; }
@@ -250,7 +223,6 @@ const unixlib_entry_t __wine_unix_call_funcs[] = ntlm_chat, ntlm_cleanup, ntlm_fork, - ntlm_check_version, };
C_ASSERT( ARRAYSIZE(__wine_unix_call_funcs) == unix_funcs_count ); @@ -310,7 +282,6 @@ const unixlib_entry_t __wine_unix_call_wow64_funcs[] = wow64_ntlm_chat, ntlm_cleanup, wow64_ntlm_fork, - ntlm_check_version, };
C_ASSERT( ARRAYSIZE(__wine_unix_call_wow64_funcs) == unix_funcs_count ); diff --git a/dlls/msv1_0/unixlib.h b/dlls/msv1_0/unixlib.h index d71b86770fb..bfa9497205e 100644 --- a/dlls/msv1_0/unixlib.h +++ b/dlls/msv1_0/unixlib.h @@ -111,6 +111,5 @@ enum ntlm_funcs unix_chat, unix_cleanup, unix_fork, - unix_check_version, unix_funcs_count, };
Piotr Caban (@piotr) commented about dlls/msv1_0/unixlib.c:
posix_spawn_file_actions_addclose( &file_actions, pipe_in[0] ); posix_spawn_file_actions_addclose( &file_actions, pipe_in[1] );
- if (posix_spawnp( &ctx->pid, params->argv[0], &file_actions, NULL, params->argv, environ ))
- if ((err = posix_spawnp( &ctx->pid, params->argv[0], &file_actions, NULL, params->argv, environ ))) { ctx->pid = -1; write( pipe_in[1], "BH\n", 3 );
How about closing pipes on unsuccessful spawn?
On Fri May 2 09:22:20 2025 +0000, Piotr Caban wrote:
How about closing pipes on unsuccessful spawn?
ntlm_cleanup() handles that, which is called on failure.
On Fri May 2 10:15:10 2025 +0000, Hans Leidekker wrote:
ntlm_cleanup() handles that, which is called on failure.
Yes, but it's uncommon to require calling cleanup function when initialization fails.
On Fri May 2 10:17:28 2025 +0000, Piotr Caban wrote:
Yes, but it's uncommon to require calling cleanup function when initialization fails.
Initialization of the context takes multiple steps. ntlm_chat() can also fail and leave a buffer behind. I'm not sure if it's better to rely on individual steps to clean up after themselves instead of having a catch-all here.
On Fri May 2 10:55:33 2025 +0000, Hans Leidekker wrote:
Initialization of the context takes multiple steps. ntlm_chat() can also fail and leave a buffer behind. I'm not sure if it's better to rely on individual steps to clean up after themselves instead of having a catch-all here.
Sure, it's your call. I'm thinking about it as: - `ntlm_fork` does initialization, if it fails context should not be created - `ntlm_chat` operates on context, so it's not responsible for clearing it in case of error - `ntlm_cleanup` cleanups the context
It's a very common pattern, like e.g. in file operations - you don't expect that failed read will close file descriptor. But you do expect that failed open will not create it.
Current code looks very strange for me. We're writing "BH" to the pipe, so I'm expecting that there may be a path that reads it but it's never read.
On Fri May 2 11:04:12 2025 +0000, Piotr Caban wrote:
Sure, it's your call. I'm thinking about it as:
- `ntlm_fork` does initialization, if it fails context should not be created
- `ntlm_chat` operates on context, so it's not responsible for clearing
it in case of error
- `ntlm_cleanup` cleanups the context
It's a very common pattern, like e.g. in file operations - you don't expect that failed read will close file descriptor. But you do expect that failed open will not create it. Current code looks very strange for me. We're writing "BH" to the pipe, so I'm expecting that there may be a path that reads it but it's never read.
Right, I see ntlm_fork() as doing part of the initialization and ntlm_chat() doing another part. ntlm_SpInitLsaModeContext() creates the context, not ntlm_fork(), so to me it makes sense to me that the former is responsible for cleanup.
Yes, the "BH" thing is strange. It can probably be removed.
-
Hans Leidekker -
Hans Leidekker (＠hans)
-
Piotr Caban (＠piotr)