Re: secur32: Getting real functionality into the NTLM provider

[Alexandre Julliard]

Kai Blin <blin(a)gmx.net> writes:
> +            unsigned char buffer[SECUR32_MAX_BUF_LEN+6000];
> +            BYTE bin[SECUR32_MAX_BUF_LEN];

You shouldn't allocate fixed size buffers on the stack, especially not
huge ones like that. You should compute the necessary size and
allocate a properly sized buffer from the heap.
> +    SEC_CHAR client_user_arg[512];
> +    SEC_CHAR client_domain_arg[512];
> +    SEC_CHAR passwd[512] = {0};

Same here, there's nothing that guarantees the buffers are the right
size; using snprintf may avoid overflows but it doesn't make the code
behave correctly.
> +SECURITY_STATUS fork_helperA(PNegoHelperA *new_helper, const char *prog,
> +        char * const argv[], char* passwd)
> +{

You should have a single fork helper that takes Unicode, you can't
pass ASCII chars to the Unix app anyway, you have to use
CP_UNIXCP. And more generally, you should do everything in Unicode.
> +    if( ( pipe(pipe_in) < 0 ) || ( pipe(pipe_out) < 0 ) )
> +    {
> +        return SEC_E_INTERNAL_ERROR;
> +    }
> +
> +    helper->helper_pid = fork();
> +
> +    helper->password = passwd;
> +
> +    if(helper->helper_pid == -1)
> +    {
> +        return SEC_E_INTERNAL_ERROR;
> +    }

You are leaking file descriptors in the error cases.
> +    if(helper->helper_pid == 0)
> +    {
> +        /* We're in the child now */
> +        close(0);
> +        close(1);
> +        
> +        dup2(pipe_out[0], 0);
> +        close(pipe_out[0]);
> +        close(pipe_out[1]);
> +
> +        dup2(pipe_in[1], 1);
> +        close(pipe_in[0]);
> +        close(pipe_in[1]);
> +
> +        execvp(prog, argv);
> +
> +        /* Whoops, we shouldn't get here. Big badaboom.*/
> +        return SEC_E_INTERNAL_ERROR;

You shouldn't return from the child, you should pass the error back to
the parent and then exit().
> +    {
> +        helper->pipe_in = fdopen(pipe_in[0], "r");
> +        close(pipe_in[1]);
> +        helper->pipe_out = fdopen(pipe_out[1], "w");
> +        close(pipe_out[0]);
> +    }

You should avoid using stdio.

-- 
Alexandre Julliard
julliard(a)winehq.org

secur32: Getting real functionality into the NTLM provider

[Kai Blin]

* Alexandre Julliard <julliard(a)winehq.org> [23/08/05, 20:39:52]:

Thanks for the comments. I see you points in all of them, apart from the
following one, where I didn't understand what you meant.
> > +    {
> > +        helper->pipe_in = fdopen(pipe_in[0], "r");
> > +        close(pipe_in[1]);
> > +        helper->pipe_out = fdopen(pipe_out[1], "w");
> > +        close(pipe_out[0]);
> > +    }
>
> You should avoid using stdio.

How would I do this avoiding stdio? ntlm_auth uses stdin and stdout to
read input and write output. This seems to be the Unix way of doing
things.

Cheers,
Kai

-- 
Kai Blin, (blin at gmx dot net)
My Bonnie looked into a gas tank,
The height of its contents to see!
She lit a small match to assist her,
Oh, bring back my Bonnie to me.

secur32: Getting real functionality into the NTLM provider

[Alexandre Julliard]

Kai Blin <blin(a)gmx.net> writes:
> * Alexandre Julliard <julliard(a)winehq.org> [23/08/05, 20:39:52]:
> > You should avoid using stdio.
>
> How would I do this avoiding stdio? ntlm_auth uses stdin and stdout to
> read input and write output. This seems to be the Unix way of doing
> things.

I mean the stdio functions like fdopen, fgets, etc.  They are not
guaranteed to be thread-safe because of the way we handle libc
threads. It's better to use normal read()/write() instead.

-- 
Alexandre Julliard
julliard(a)winehq.org