[FFmpeg-devel] [PATCH] Fix MSVC warnings about possible value truncation.

[wm4]

On Tue, 2 Sep 2014 23:43:33 +0200
Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:

> On Tue, Sep 02, 2014 at 01:13:27PM -0700, Peter Kasting wrote:
> > On Sat, Aug 30, 2014 at 2:21 AM, wm4 <nfxjfg at googlemail.com> wrote:
> > > I'd
> > > expect it rather to hide bugs than to expose them. For example, it
> > > could make a static analyzer with value range analysis stop working,
> > > because the casts will basically throw a wrench into its algorithm.
> > 
> > 
> > I can't understand how the sorts of casts that would be introduced here.
> > I'm suggesting would harm static analysis.  If we're assigning a value to a
> > variable of type T, then "T var = x;" and "T var = (T)x;" have the same
> > effect, and a static analyzer ought to treat them identically.
> 
> The problem is that we have nothing that ensures the form
> T var = (T)x;
> over
> T2 var = (T)x;
> (where T2 is a wider type than T for example).
> Depending on the static analyzer you can make it ignore
> T var = x;
> but it will start warning again once the type of var or x changes.
> 
> 
> +                // !!! Is this cast safe?
> +                sub->end_display_time = (uint32_t)av_rescale_q(avpkt->duration,
> +                                                               avctx->pkt_timebase, ms);
> 
> Don't really see anything much better to do.
> You could clamp it to e.g. INT_MAX, but not convinced it
> would get any better by that.

Well, if you have an excessive duration, that would help to get the
correct behavior. It's really easy to write a very large timestamp into
a subtitle file.

> 
> +            // !!! Are these casts safe?
> +            s->base_matrix[0][i]        = (uint8_t)vp31_intra_y_dequant[i];
> +            s->base_matrix[1][i]        = (uint8_t)vp31_intra_c_dequant[i];
> +            s->base_matrix[2][i]        = (uint8_t)vp31_inter_dequant[i];
> 
> That one is answered by a simple look in the table: None are negative, so yes.
> No idea if there is a specific reason they use signed types.
> Considering this seems to be the only place they are used, it probably is
> a "bug" in sofar as the declared types of the tables are stupid.
> 
>      secs %= 86400;
> -    tm->tm_hour = secs / 3600;
> +    tm->tm_hour =(int)(secs / 3600);
>      tm->tm_min = (secs % 3600) / 60;
> -    tm->tm_sec =  secs % 60;
> +    tm->tm_sec = secs % 60;
> 
> The second part breaks the alignment. And the first one the compiler in theory could figure out on its own...
> 
> +// !!! Should |pts_num| and |pts_den| be uint64_t?
>  void avpriv_set_pts_info(AVStream *s, int pts_wrap_bits,
>                           unsigned int pts_num, unsigned int pts_den);
> 
> I can only hope that nobody has/will come up with some insanity where
> that would be useful.

Actually, going through the patch, it might help with some overflow
checks in demuxers. We could just clamp the input to sane values (so
invalid files don't break all applications, without needing too many
checks and all that).

> [...]