On Wed, Oct 28, 2015 at 6:07 PM, Derek Buitenhuis
<derek.buitenh...@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
