[FFmpeg-devel] [PATCH] avformat: implement SChannel SSP TLS protocol

[Hendrik Leppkes]

On Wed, Oct 28, 2015 at 6:07 PM, Derek Buitenhuis
<derek.buitenhuis at gmail.com> wrote:
> Enjoy my half-assed / useless review.
>
>> +#ifndef SECBUFFER_ALERT
>> +#define SECBUFFER_ALERT                17
>> +#endif
>
> Why?

I think it was needed on some configuration, I'll verify.

>
>> +    SecPkgContext_StreamSizes Sizes;
>
> Accidental capital?
>
>> +    if (c->enc_buf == NULL) {
>> +        c->enc_buf_offset = 0;
>> +        c->enc_buf_size = SCHANNEL_INITIAL_BUFFER_SIZE;
>> +        ret = av_reallocp(&c->enc_buf, c->enc_buf_size);
>> +        if (ret < 0)
>> +            goto fail;
>
> I can never remember if _close() is guaranteed to be called. I'll assume yes.
>
>> +    while (1)
>> +    {
>> +        if (c->enc_buf_size - c->enc_buf_offset < SCHANNEL_FREE_BUFFER_SIZE) {
>
> Can enc_buf_offset ever end up bigger? e.. if data keeps being read, and you keep
> getting SEC_E_INCOMPLETE_MESSAGE.

Bigger than what, buf_size? it should grow that as necessary when data
is received.

>
>> +        /* input buffers */
>> +        InitSecBuffer(&inbuf[0], SECBUFFER_TOKEN, av_malloc(c->enc_buf_offset), c->enc_buf_offset);
>
> Is this a unchecked malloc, or is it passed into inbuf[0].pvBuffer?

Yes this ends up in inbuf[0].pvBuffer which is then checked.
I could make it explicit before if people like.

>
>> +
>> +        sspi_ret = InitializeSecurityContext(&c->cred_handle, &c->ctxt_handle, s->host, c->request_flags,
>> +                                             0, 0, &inbuf_desc, 0, NULL, &outbuf_desc, &c->context_flags,
>> +                                             &c->ctxt_timestamp);
>> +        av_freep(&inbuf[0].pvBuffer);
>
> Double checking: is this safe?

Since this is the buffer we av_malloc'ed before, yes.

>
>
>> +    /* SChannel Options */
>> +    memset(&schannel_cred, 0, sizeof(schannel_cred));
>
> SCHANNEL_CRED schannel_cred = { 0 };
>
>> +cleanup:
>> +    size = len < c->dec_buf_offset ? len : c->dec_buf_offset;
>> +    if (size) {
>> +        memcpy(buf, c->dec_buf, size);
>> +        memmove(c->dec_buf, c->dec_buf + size, c->dec_buf_offset - size);
>> +        c->dec_buf_offset -= size;
>> +
>> +        return size;
>> +    }
>> +
>> +    if (ret == 0 && !c->connection_closed)
>> +        ret = AVERROR(EAGAIN);
>> +
>> +    return ret < 0 ? ret : 0;
>> +
>> +fail:
>> +    return ret;
>
> No cleanup in case of failure?

fail is only used for serious unrecoverable failures, like socket read
failure or mem allocation failure. It could still call that code, I
suppose, just in case some valid data is stuck in the decrypted
buffer.

>
> - Derek
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel