[MPlayer-dev-eng] [PATCH] vf_screenshot, output filename corresponding to the played file

[Michael Niedermayer]

On Mon, Nov 10, 2008 at 03:01:50PM +0300, Ruslan Savchenko wrote:
> On Mon, Nov 10, 2008 at 11:43 AM, Attila Kinali <attila at kinali.ch> wrote:
> 
> > On Sun, 9 Nov 2008 22:03:07 +0300
> > "Ruslan Savchenko" <ruslan.savchenko at gmail.com> wrote:
[...]
> > +    int hour = (int) (pts/3600);
> > > +    int min = ((int) (pts/60)) % 60;
> > > +    int sec = ((int) pts) % 60;
> > > +    int msec = (int) (pts*100) % 100;
> >
> > Are you sure that pts can never overflow an int?
> > (IIRC pts is here first converted to int, then the division/modulo
> > is applied). Beside, uint would be the appropriate type here,
> > as time values can never become negative.
> 
> 
> 2^15 is 9 hours. Not enough i suppose. Switched to unsigned long.

int in posix is at least 32 bit


> 
> Are you sure that a trailing '\0' is written even in case snprintf
> > trunkates its output? (A quick look into the manpage didnt reveal anything)
> >
> 
> I believe ISO/IEC 9899:1999 says '\0' is always written by snprintf. But
> anyway,
> in current patch most of snprntfs are exchanged by sprintf because enough
> memory is allocated right before the operation.

i dont think this is a good idea

[...]

> @@ -92,15 +100,69 @@
>      else return 0;
>  }
>  
> -static void gen_fname(struct vf_priv_s* priv)
> +#define TIMESTAMP_LENGTH 12
> +#define SHOT_FNAME_LENGTH 13
> +
> +static void gen_fname(struct vf_priv_s* priv, double pts)
>  {
> -    do {
> -	snprintf (priv->fname, 100, "shot%04d.png", ++priv->frameno);
> -    } while (fexists(priv->fname) && priv->frameno < 100000);
> -    if (fexists(priv->fname)) {
> -	priv->fname[0] = '\0';
> -	return;
[...]
> +    char *s;
> +    char tstamp[TIMESTAMP_LENGTH];
> +    unsigned long hour = (unsigned long) (pts/3600);
> +    unsigned long min  = (unsigned long) (pts/60) % 60;
> +    unsigned long sec = (unsigned long) pts % 60;
> +    unsigned long msec = (unsigned long) (pts*100) % 100;
> +
> +    if (priv->fnametype == 0) {
> +        /* allocate space to handle "shotXXXX.png" filenames */
> +        priv->fname = malloc(SHOT_FNAME_LENGTH);
> +        do {
> +            sprintf (priv->fname, "shot%04d.png", ++priv->frameno);
> +            } while (fexists(priv->fname) && priv->frameno < 100000);

what is the < 100000 check supposed to be?
but i see now this code was already there before, but you replace the
snprintf() by sprintf and VERY significantly reduce the array size so
the code will write over the allocated space.

This is a nice example why reindenting code in functional patches IS a
bad idea.



> +        if (fexists(priv->fname)) {
> +	          priv->fname[0] = '\0';
> +	          return;
> +        }
>      }
> +    else if ((priv->fnametype == 1) || (priv->fnametype == 2)) {
> +        if (priv->fnametype == 1)
> +            snprintf(tstamp, TIMESTAMP_LENGTH, "%02lu:%02lu:%02lu.%02lu", hour, min, sec, msec);
> +        else if (priv->fnametype == 2)
> +            snprintf(tstamp, TIMESTAMP_LENGTH, "%02lu-%02lu-%02lu.%02lu", hour, min, sec, msec);

TIMESTAMP_LENGTH is clearly too small for more han 99 hours


> +            
> +        if (!priv->fnamebase) {
> +            if (strstr(filename, "://") != NULL) {

> +                priv->fnamebase = malloc(SHOT_FNAME_LENGTH);
> +                strcpy(priv->fnamebase, "shot");

strdup()


> +            }
> +            else {
> +                s = strrchr(filename, '/');
> +                if (s == NULL) 
> +                    s = filename;
> +                else
> +                    s++;
> +                priv->fnamebase = malloc(strlen(s) + 1);
> +                strcpy(priv->fnamebase, s);
> +            }
> +        }
> +        /* 8 is length of ".[].png" plus '\0' */

> +        priv->fname = malloc(strlen(priv->fnamebase) + strlen(tstamp) + 8);
> +        sprintf(priv->fname, "%s.[%s].png", priv->fnamebase, tstamp);

interger oveflows leading to segfaults or exploits


[...]
> +        vf->priv=malloc(sizeof(struct vf_priv_s));
> +        memset(vf->priv, 0, sizeof(struct vf_priv_s));

calloc()


besides these, i seriously douubt all the *alloc() in this patch is needed

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

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- 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/mplayer-dev-eng/attachments/20081111/92b9b386/attachment.pgp>