<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<meta http-equiv="Content-Style-Type" content="text/css" />
<meta name="generator" content="pandoc" />
<title></title>
<style type="text/css">code{white-space: pre;}</style>
</head>
<body>
<p>Working on a new set of patches that heavily refactors the implementation, please view this thread (and the patches inside) as obsolete.</p>
<p>On 2016-10-29 00:21, Filip Roséen wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> Hi,
Seems like today is the day where I make a lot of typos and silly
mistakes.. right after sending the patch I realized that the
bitfiddling is of course wrong.
I deeply apologize, and perhaps I could mention that I got news of
illness recently in the family - so my mind is somewhat scattered at
the moment.
-----------------------------------------------------------------------
In order to align something to a boundrary of 2, `(x + 1) & ~1` does
the right thing, `( x + 1 ) ^ 1` does not make any sense whatsoever.
See attached patch for a correct implementation, and the below for a
difference between the previous submitted, and the new one.
- ssize_t i_chunk_size = ( i_data_size + 8 + 1 ) ^ 1;
+ ssize_t i_chunk_size = ( i_data_size + 8 + 1 ) & ~1;
Best regards,\
Filip
-----------------------------------------------------------------------
On 2016-10-29 00:10, Filip Roséen wrote:</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> The previous implementation could potentially overflow the uint32_t
holding the data-size when adding the size of the mandatory header, in
order to consume the entire chunk. If that happened we could end up in
an infinite loop (given that we are not guaranteed to make progress).
These changes fixes the issue by introducing another variable that is
only used for the purpose of storing the chunk (total) size.
fixes #17562</code></pre>
</blockquote>
</blockquote>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> From b61e3f77031cd090dd46841bfa9b8244dcfb0d4d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Filip=20Ros=C3=A9en?= <filip@atch.se>
Date: Sat, 29 Oct 2016 00:04:23 +0200
Subject: [PATCH] demux/aiff: fix integer overflow leading to infinite loop
The previous implementation could potentially overflow the uin32_t
holding the data-size when adding the size of the mandatory header (in
order to consume the entire chunk), and if that happened we could end
up in an infinite loop (given that we are not making progress).
These changes fixes the issue by introducing another variable that is
only used for the purpose of storing the chunk (total) size.
fixes #17562
--
I will clean-up the implementation shortly, but thought I would
address this immidiate issue as soon as possible.
The demuxer can be made a lot simpler, and a lot safer.
---
modules/demux/aiff.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/modules/demux/aiff.c b/modules/demux/aiff.c
index bdd0308..9631fdb 100644
--- a/modules/demux/aiff.c
+++ b/modules/demux/aiff.c
@@ -123,14 +123,13 @@ static int Open( vlc_object_t *p_this )
for( ;; )
{
- uint32_t i_size;
-
if( vlc_stream_Peek( p_demux->s, &p_peek, 8 ) < 8 )
goto error;
- i_size = GetDWBE( &p_peek[4] );
+ uint32_t i_data_size = GetDWBE( &p_peek[4] );
+ ssize_t i_chunk_size = ( i_data_size + 8 + 1 ) & ~1;
- msg_Dbg( p_demux, "chunk fcc=%4.4s size=%d", p_peek, i_size );
+ msg_Dbg( p_demux, "chunk fcc=%4.4s size=%" PRIu32, p_peek, i_data_size );
if( !memcmp( p_peek, "COMM", 4 ) )
{
@@ -152,7 +151,7 @@ static int Open( vlc_object_t *p_this )
goto error;
p_sys->i_ssnd_pos = vlc_stream_Tell( p_demux->s );
- p_sys->i_ssnd_size = i_size;
+ p_sys->i_ssnd_size = i_data_size;
p_sys->i_ssnd_offset = GetDWBE( &p_peek[8] );
p_sys->i_ssnd_blocksize = GetDWBE( &p_peek[12] );
@@ -165,11 +164,8 @@ static int Open( vlc_object_t *p_this )
break;
}
- /* Skip this chunk */
- i_size += 8;
- if( (i_size % 2) != 0 )
- i_size++;
- if( vlc_stream_Read( p_demux->s, NULL, i_size ) != (int)i_size )
+ /* consume chunk */
+ if( vlc_stream_Read( p_demux->s, NULL, i_chunk_size ) != i_chunk_size )
{
msg_Warn( p_demux, "incomplete file" );
goto error;
--
2.10.1
</code></pre>
</blockquote>
</body>
</html>