diff mbox

[FFmpeg-devel] avformat/tls_schannel: Fix use of uninitialized variable

Message ID CA+op6QZfb0nHZxoVjej0UozicoOFkT5soT0gsQNX6wArt_2tEQ@mail.gmail.com
State Superseded
Headers show

Commit Message

Paweł Wegner Aug. 2, 2018, 6:46 p.m. UTC 
Fixes: runtime error: passing uninitialized value to FreeContextBuffer
causes a crash
---
 libavformat/tls_schannel.c | 2 ++
 1 file changed, 2 insertions(+)

         ret = av_reallocp(&c->enc_buf, SCHANNEL_INITIAL_BUFFER_SIZE);
--
2.10.1

Comments

Carl Eugen Hoyos Aug. 3, 2018, 12:04 a.m. UTC | #1 
2018-08-02 20:46 GMT+02:00, Paweł Wegner <pawel.wegner95@gmail.com>:
> Fixes: runtime error: passing uninitialized value to FreeContextBuffer
> causes a crash
> ---
>  libavformat/tls_schannel.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/libavformat/tls_schannel.c b/libavformat/tls_schannel.c
> index 065dccb..6953008 100644
> --- a/libavformat/tls_schannel.c
> +++ b/libavformat/tls_schannel.c
> @@ -154,6 +154,8 @@ static int tls_client_handshake_loop(URLContext *h, int
> initial)
>      SecBufferDesc inbuf_desc;
>      int i, ret = 0, read_data = initial;
>
> +    memset(outbuf, 0, sizeof(outbuf));

Is the memset necessary or is an initialization of the variable sufficient?

Carl Eugen
Paweł Wegner Aug. 3, 2018, 5:32 p.m. UTC | #2 
One could copy the initialization to the top from the while loop:
  init_sec_buffer(&outbuf[0], SECBUFFER_TOKEN, NULL, 0);
  init_sec_buffer(&outbuf[1], SECBUFFER_ALERT, NULL, 0);
  init_sec_buffer(&outbuf[2], SECBUFFER_EMPTY, NULL, 0);
  init_sec_buffer_desc(&outbuf_desc, outbuf, 3);

But memset is shorter. Current code will crash when there is any failure
before this initialization.
Paweł Wegner Aug. 3, 2018, 6:28 p.m. UTC | #3 
One could copy the initialization to the top from the while loop:
  init_sec_buffer(&outbuf[0], SECBUFFER_TOKEN, NULL, 0);
  init_sec_buffer(&outbuf[1], SECBUFFER_ALERT, NULL, 0);
  init_sec_buffer(&outbuf[2], SECBUFFER_EMPTY, NULL, 0);
  init_sec_buffer_desc(&outbuf_desc, outbuf, 3);

But memset is shorter. Current code will crash when there is any failure
before this initialization.
Paweł Wegner Aug. 16, 2018, 12:16 p.m. UTC | #4 
Could someone merge it?
Carl Eugen Hoyos Aug. 17, 2018, 9:15 a.m. UTC | #5 
2018-08-03 20:28 GMT+02:00, Paweł Wegner <pawel.wegner95@gmail.com>:

> One could copy the initialization to the top from the while loop:
>   init_sec_buffer(&outbuf[0], SECBUFFER_TOKEN, NULL, 0);
>   init_sec_buffer(&outbuf[1], SECBUFFER_ALERT, NULL, 0);
>   init_sec_buffer(&outbuf[2], SECBUFFER_EMPTY, NULL, 0);
>   init_sec_buffer_desc(&outbuf_desc, outbuf, 3);

I was thinking about "SecBuffer outbuf = { 0 };
Does that work?

Carl Eugen
diff mbox

Patch

diff --git a/libavformat/tls_schannel.c b/libavformat/tls_schannel.c
index 065dccb..6953008 100644
--- a/libavformat/tls_schannel.c
+++ b/libavformat/tls_schannel.c
@@ -154,6 +154,8 @@  static int tls_client_handshake_loop(URLContext *h, int
initial)
     SecBufferDesc inbuf_desc;
     int i, ret = 0, read_data = initial;

+    memset(outbuf, 0, sizeof(outbuf));
+
     if (c->enc_buf == NULL) {
         c->enc_buf_offset = 0;