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

[Reimar Döffinger]

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.

+            // !!! 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.

+        if (ffio_limit(pb, (int)length) != length)

This one actually looks kind of like a bug, the cast might invoke
undefined behaviour.

@@ -1500,7 +1500,7 @@ static void matroska_add_index_entries(MatroskaDemuxContext *matroska)
 {
     EbmlList *index_list;
     MatroskaIndex *index;
-    int index_scale = 1;
+    uint64_t index_scale = 1;

That looks like it will be a really expensive change,
division by a 64 bit value isn't fast.
In general, the nicer way to avoid the matroska issues and saving
a bit of memory would be to add a EBML_UINT32 (dummy) type.

-static int matroska_aac_sri(int samplerate)
+static int matroska_aac_sri(double samplerate)

I don't think this is equivalent at all.
Not sure if it is better or worse though, we seem to switch between doing
int and doing float comparisons.

-        avpriv_set_pts_info(st, 64, matroska->time_scale * track->time_scale,
+        avpriv_set_pts_info(st, 64, (unsigned int)(matroska->time_scale * track->time_scale),
                             1000 * 1000 * 1000);    /* 64 bit pts in ns */

I see where the other question came from.
That looks a bit annoying, won't work for time_scale values > 4s??
Not that I think anyone cares.


--- a/libavformat/riffdec.c
+++ b/libavformat/riffdec.c
@@ -185,7 +185,7 @@ int ff_read_riff_info(AVFormatContext *s, int64_t size)
     while ((cur = avio_tell(pb)) >= 0 &&
            cur <= end - 8 /* = tag + size */) {
         uint32_t chunk_code;
-        int64_t chunk_size;
+        unsigned int chunk_size;

That seems risky to me.
We seem to have the right checks right now, but just one mistake
and we have an integer overflow in a malloc argument and
a huge security hole if we using a 32 bit type here.


         if (bitrate >= 0 && bitrate <= INT_MAX)
-            ic->bit_rate = bitrate;
+            ic->bit_rate = (int)bitrate;

I'd kind of think the compilers should manage at least these trivial cases...

--- a/libavformat/wavdec.c
+++ b/libavformat/wavdec.c
@@ -61,7 +61,7 @@ typedef struct WAVDemuxContext {
 
 #if CONFIG_WAV_DEMUXER
 
-static int64_t next_tag(AVIOContext *pb, uint32_t *tag)
+static unsigned int next_tag(AVIOContext *pb, uint32_t *tag)
 {
     *tag = avio_rl32(pb);
     return avio_rl32(pb);
@@ -76,10 +76,10 @@ static int64_t wav_seek_tag(WAVDemuxContext * wav, AVIOContext *s, int64_t offse
 }
 
 /* return the size of the found tag */
-static int64_t find_tag(WAVDemuxContext * wav, AVIOContext *pb, uint32_t tag1)
+static unsigned int find_tag(WAVDemuxContext * wav, AVIOContext *pb, uint32_t tag1)
 {
     unsigned int tag;
-    int64_t size;
+    unsigned int size;
 
     for (;;) {
         if (avio_feof(pb))

These seem reasonable




-            int level = e->param[1] ? av_clip(eval_expr(p, e->param[1]), INT_MIN, INT_MAX) : AV_LOG_INFO;
+            int level = e->param[1] ? (int)av_clip64((int64_t)eval_expr(p, e->param[1]), INT_MIN, INT_MAX) : AV_LOG_INFO;

?? I think I've seen another one like it. Were they meant to be av_clipf/av_clipd?


-            int idx= av_clip(eval_expr(p, e->param[0]), 0, VARS-1);
-            uint64_t r= isnan(p->var[idx]) ? 0 : p->var[idx];
+            int idx= av_clip((int)eval_expr(p, e->param[0]), 0, VARS-1);
+            uint64_t r= isnan(p->var[idx]) ? 0 : (uint64_t)p->var[idx];
             r= r*1664525+1013904223;
-            p->var[idx]= r;
+            p->var[idx]= (double)r;

Can this even work correctly?? I think we'd want to mask that to what double can store without losing precision.