[MPlayer-dev-eng] [patch] play and seek files inside multivolume uncompressed rar files

[Reimar Döffinger]

A first review, it is unlikely to have caught everything, and almost
completely ignores cosmetics.

On Tue, Dec 08, 2009 at 08:12:02PM +0100, Emiel Neggers wrote:
> +static int control(stream_t *s, int cmd, void *arg) {
> +    struct stream_priv_s* p = s->priv;
> +    switch(cmd) {
> +        case STREAM_CTRL_GET_SIZE: {
> +            *((long long unsigned int*)arg) = rar_size(p->rar);

That's just wrong, the type is off_t, and will cause crashes on a
plain MinGW where off_t is 32 bit (though STREAM_CTRL_GET_SIZE is not really used
anywhere except the LAVF muxer).

> +            return 1;
> +        }
> +        case STREAM_CTRL_RESET: {
> +            rar_seek(p->rar, 0);
> +        }

None of the other streams seem to need that.
Also, the {} are useless in both cases.

> +static int open_rar(stream_t *stream,int mode, void* opts, int* file_format) {
> +    struct stream_priv_s* p;
> +    char *filename;
> +
> +    // get filename from option 'url'
> +    if (((struct stream_priv_s*)opts)->filename)
> +        filename = ((struct stream_priv_s*)opts)->filename;
> +    else if (((struct stream_priv_s*)opts)->filename2)
> +        filename = ((struct stream_priv_s*)opts)->filename2;
> +    else
> +        filename = NULL;

Why aren't you using the p variable like the other stream modules since you have
it already instead of all those ugly casts?

> +    // do not handle any non-rar file
> +    if(!filename || !rar_has_rar_filename(filename)) {

If the user requested this stream module, you should honor that request and
try to something sensible even if the extension isn't .rar

> +        m_struct_free(&stream_opts,opts);
> +        return STREAM_ERROR;

Do the freeing in one place, using goto if necessary. If you have more than
one place where things are freed that basically guarantees memory leaks
in the long term.

> +    p = calloc(1,sizeof(struct stream_priv_s));
[...]
> +    m_struct_free(&stream_opts,opts);

Why? opts is already a perfectly fine "struct stream_priv_s", why
allocate a new one and discard the one you already have?

> +#include <ctype.h>

If you need that you are most likely doing something that won't work
reliably and thus is wrong.
In this particular case, while it happens to work with MPlayer for now,
isdigit is not at all what you want, you want to know if the value is
0 - 9 in ASCII, or does your code handle Chinese or Japanese digits
correctly (I admit it can not really happen like this with your code,
still isdigit just isn't the correct function to use)?

> +#include <math.h>

Hmm, what is that used for?

> +#define UINT32(b) ( (*((b)+3))<<24 | *(((b)+2))<<16 | *(((b)+1))<<8 | *(((b)+0)) )

AV_RL32 from libavutil

> +int _rar_init_part(rar_archive *rar) {

Please no leading underscores for name.
Add mp_ if there might be a namespace issue.

> +    unsigned char buf[RAR_HEADER_SIZE];

Use uint8_t for data and char (even unsigned) only for strings.

> +    unsigned int hs;

Not a great name.

> +    size = (buf[i+0x06] << 8) | buf[i+0x05]; // size of file_header block

AV_RL16

> +    if(stat(filename, &buf) < 0) {
> +        mp_msg(MSGT_STREAM,MSGL_V,"[rar] failed to open part %s: %s\n", 
> +            filename, strerror(errno));

What's the point? Why does it matter if stat fails or not, it only
matters if open() works or not and the error message is wrong anyway.

> +    if(rar->fd != -1) {
> +        close(rar->fd);
> +        rar->fd = -1;
> +    }
> +
> +    rar->fd = open(filename, O_RDONLY|O_BINARY);

Setting fd to -1 if you set it again right away seems a bit pointless.

> +    if(rar->fd < 0) {
> +        mp_msg(MSGT_STREAM,MSGL_V,"[rar] failed to open part %s: %s\n", 
> +            filename, strerror(errno));
> +        return rar->fd;

I don't think it is a good idea to return rar->fd..
You might also consider to check for == -1 explicitly, while
open is indeed not allowed to return something < 0 and != -1, it
does for WinCE (and there it means the file was opened successfully).

> +    // generate filename for the given volume number
> +    if(rar->new_naming_scheme) {
> +    	len = strlen( rar->filename ) + 10 + rar->num_length;

Given this code:
> while(isdigit(*(t++))) rar->num_length++;
I fear this calculation might overflow given the right (about 2GB large)
file name.
Since you use snprintf that shouldn't be exploitable but just using
some constant might be better.

> +        // test if it is marked as a rar archive
> +        if(len < 61 || buf[0] != 'R' || buf[1] != 'a' || buf[2] != 'r' || buf[3] != '!' || 

Einther of
memcmp(buf, "Rar!", 4)
strncmp(buf, "Rar!", 4)
AV_RN32(buf) != AV_RN32("Rar!")
is easier to read.

> +        // read file header
> +        i = 0x14; // start of file header

You've been using 0x14 +... a few times above already, this probably should be reordered a bit.

> +        size = (buf[i+0x06] << 8) | buf[i+0x05]; // size of file_header block

AV_RL16(buf + i + 5);

> +        if( buf[i+0x04] & 0x1 ) {

And I think it would be easier to follow if you processed the fields in the order
they are in memory where easy to do.

> +            if((p = strrchr(rar->filename, '.')) != NULL && strlen(p) > 5 &&
> +                            (p = strstr(p, ".part")) != NULL && isdigit(*(p+5))) {

The strlen check seems pointless.
And *(p+5) is a complicated way for writing p[5].
Also I think this would be easier to understand as
p = strrchr(rar->filename, '.');
if (p)
    p = strstr(p, ".part");
if (!p || p[5] < '0' || p[5] > '9')
   error
normal processing

> +    long int res = 0;
> +    long int pos = lseek(rar->fd, 0L, SEEK_CUR);

Simple rule: "long int" is always wrong.
off_t is the right type, though int64_t is ok as well.

> +    if( (pos + count) > (rar->cur_file_size - rar->trailing_bytes) ) {
> +        // read last part of this file, and start of next file
> +        res = read(rar->fd, buf, rar->cur_file_size - rar->trailing_bytes - pos);

Using the same expression makes it easier to read/understand and much easier
to verify it has no security issues.
if (rar->cur_file_size - rar->trailing_bytes - pos < count)

> +        res += read(rar->fd, (char*)buf + res, count - res);

That's not going to do the right thing if read fails.
It also won't do the right thing if "count - res" is still larger
than the content of the next file (try creating rar archives with e.g.
1 kB per part and it should become obvious).

> +long long int rar_seek(rar_archive *rar, long long int offset) {
> +    int p;
> +    long long int o = offset;
> +    long long int spp = rar->size_per_part;

Please use int64_t, that's just more "reliable"/future proof/shorter
to write.
This of course applies everywhere.

> +    long int res;

And above comment for "long int" applies here as well of course.

> +    p = offset / rar->size_per_part;
> +
> +    if(_rar_openpart(rar, p) < 0) {
> +        return -1;
> +    }
> +
> +    o = offset - (spp * p) + rar->header_size;

I guess
p = offset / rar->size_per_part;
o = offset % rar->size_per_part + rar->header_size;
would be more obvious (on quite a few CPUs it would even be faster,
not that it matters here though).

> +    res = o & 0xffffffff;

What??