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

[Derek Buitenhuis]

Enjoy my half-assed / useless review.

> +#ifndef SECBUFFER_ALERT
> +#define SECBUFFER_ALERT                17
> +#endif

Why?

> +    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.

> +        /* 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?

> +
> +        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?


> +    /* 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?

- Derek