713310 - Clang Static Analysis: Assigned value is garbage or undefined in media/libnestegg/src/nestegg.c

Status: NEW

(Core :: Audio/Video: Playback, defect)

Description

[Christian Holler (:decoder)]

The following report (in the URL field) has been generated by static analysis using Clang.

It would be good if someone familiar with the particular code could check if

- this is really a bug or a false positive
- and/or if it makes sense to adjust the code (even if there is not a real bug present, e.g. by adding a missing initialization).


In this particular report, the analyzer seems to assume that | count = *p++ + 1; | in line 1737 can evaluate to 1 such that the | while(--count) | loop is not executed. At the same time it assumes | item | can be larger than 0 and therefore, access | sizes[item]; | in line 1757 is invalid.

Comment 1

[Matthew Gregan [:kinetik]]

I was looking at this (and the other warning in the same function) on Thursday, and it's either a false positive or I don't understand what the analyzer is complaining about.

It's possible that |item| may be >= |count| and therefore access an invalid part of |sizes| if the caller passes in an invalid value, but they are expected to pass only values from 0..count-1, as queried via nestegg_track_codec_data_count.  It wouldn't hurt to add a check that |item < count|, but doing this didn't silence the analyzer's warnings.

Comment 2

[Christian Holler (:decoder)]

(In reply to Matthew Gregan [:kinetik] from comment #1)
> It wouldn't hurt to add a check that |item < count|, but doing this didn't silence the analyzer's warnings.


What kind of check did you add there and where? I can give it a try myself then and then investigate why it doesn't silence the warning.

Comment 3

[Matthew Gregan [:kinetik]]

I'm using clang version 3.1 (trunk 147127) for this.

sizes[] is initialized from [0, count), so adding a check that item is [0, count) was my first thought:


   if (count > 3)
     return -1;
 
+  if (item >= count)
+    return -1;
+
   i = 0;
   total = 0;
   while (--count) {


But that's not sufficient to silence the warnings.  Next, I added a check that item is within the bounds of sizes[] (even though this is already established by the added check and the existing range check on count):


   if (count > 3)
     return -1;
 
+  if (item >= count)
+    return -1;
+
+  if (item >= 3)
+    return -1;
+
   i = 0;
   total = 0;
   while (--count) {


The only thing I could find that would silence the warnings was:


   if (count > 3)
     return -1;
 
+  if (item >= 3)
+    return -1;
+
+  for (i = 0; i < 3; ++i)
+    sizes[i] = 0;
+
   i = 0;
   total = 0;
   while (--count) {


...which shouldn't be necessary, at least once a check is added that verifies item <= count.

Comment 4

[Timothy B. Terriberry (:derf)]

(In reply to Matthew Gregan [:kinetik] from comment #3)
> ...which shouldn't be necessary, at least once a check is added that
> verifies item <= count.

You can also add checks like if(foo)__builtin_unreachable(); Those telll the optimizer that foo must be false, and is a decent way to silence clang warnings.