[FFmpeg-devel] [PATCH][RFC] nsv seeking

[Michael Niedermayer]

On Mon, Apr 20, 2009 at 08:03:40PM +0530, Jai Menon wrote:
> On Mon, Apr 20, 2009 at 6:24 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Sun, Apr 19, 2009 at 04:46:48PM +0530, Jai Menon wrote:
> >> On 4/18/09, Fran?ois Revol <revol at free.fr> wrote:
> > [...]
> >> @@ -348,12 +349,22 @@ static int nsv_parse_NSVf_header(AVFormatContext *s, AVFormatParameters *ap)
> >> ? ? ?PRINT(("NSV got infos; filepos %"PRId64"\n", url_ftell(pb)));
> >>
> >> ? ? ?if (table_entries_used > 0) {
> >> + ? ? ? ?int i;
> >> ? ? ? ? ?nsv->index_entries = table_entries_used;
> >> ? ? ? ? ?if((unsigned)table_entries >= UINT_MAX / sizeof(uint32_t))
> >> ? ? ? ? ? ? ?return -1;
> >> - ? ? ? ?nsv->nsvs_file_offset = av_malloc(table_entries * sizeof(uint32_t));
> >> -#warning "FIXME: Byteswap buffer as needed"
> >> - ? ? ? ?get_buffer(pb, (unsigned char *)nsv->nsvs_file_offset, table_entries * sizeof(uint32_t));
> >> + ? ? ? ?nsv->nsvs_file_offset = av_malloc((unsigned)table_entries_used * sizeof(uint32_t));
> >> +
> >> + ? ? ? ?for(i=0;i<table_entries_used;i++)
> >> + ? ? ? ? ? ?nsv->nsvs_file_offset[i] = get_le32(pb) + size;
> >
> > exploitable
> 
> I can't think of any feasible attack vector here. Could you be a little verbose.

table_entries_used * 4 overflows


> 
> >> +
> >> + ? ? ? ?if(table_entries > table_entries_used &&
> >> + ? ? ? ? ? get_le32(pb) == MKTAG('T','O','C','2')) {
> >> + ? ? ? ? ? ?nsv->nsvs_timestamps = av_malloc((unsigned)table_entries_used*sizeof(uint32_t));
> >> + ? ? ? ? ? ?for(i=0;i<table_entries_used;i++) {
> >> + ? ? ? ? ? ? ? ?nsv->nsvs_timestamps[i] = get_le32(pb);
> >> + ? ? ? ? ? ?}
> >> + ? ? ? ?}
> >> ? ? ?}
> >>
> >> ? ? ?PRINT(("NSV got index; filepos %"PRId64"\n", url_ftell(pb)));
> >
> > the variable names table_entries and table_entries_used should be
> > changed
> 
> Hmm...there is a lot of possibility in that file for cleanup. Could
> that be tackled separately?

sure


> 
> > [...]
> >> @@ -700,9 +726,10 @@ static int nsv_read_close(AVFormatContext *s)
> >> ?/* ? ? int i; */
> >> ? ? ?NSVContext *nsv = s->priv_data;
> >>
> >> - ? ?if (nsv->index_entries)
> >> + ? ?if (nsv->index_entries) {
> >
> > superflous i suspect
> 
> Yeah, that should be an av_freep, but as I said, lot of possibility for cleanup.

you change the if() alraedy , you can as well remove it

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090420/11c7b404/attachment.pgp>