Re: secur32: Getting real functionality into the NTLM provider

[Alexandre Julliard]

Kai Blin  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.

secur32: Getting real functionality into the NTLM provider

[Kai Blin]

* Alexandre Julliard  [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

secur32: Getting real functionality into the NTLM provider

[Alexandre Julliard]

Kai Blin  writes:
> - Alexandre Julliard  [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.