search

hidefromkgb / gif_load

A slim, fast and header-only GIF loader written in C
80 stars 6 forks source link

Static code analyzer warning #8

Closed wcout closed 5 years ago

wcout commented 5 years ago

I've probed the clang analyzer on the code and it gives one warning. As the code is .. hm .. complex .. I can't judge if this is a valid one, so I simply show it here:

report.zip

hidefromkgb commented 5 years ago

Seems like I broke the Clang analyzer! Yay…

  1. [50] cannot be false right away, as ctbl equals at least 4 (and at most 256) at the start, see line 102.
  2. code gets initialized with exactly ctbl zero values, starting at the address of code[0], and then gets appended 1 element each SP/MP iteration, until either its size (and the value of ctbl which drives that process) equals GIF_CLEN or it gets invalidated, see line 118.
  3. prev never exceeds ctbl between lines 109 and 130, as prev equals 0 at the start and then gets its value from curr at each iteration, whereas curr exceeding ctbl terminates the function with the return value of –5 (see line 141), before prev ever gets a chance to become greater than ctbl.
  4. when ctbl reaches GIF_CLEN and thus cannot further expand code, its corresponding mask prevents curr from becoming greater than GIF_CLEN – 1, see line 118.

Maybe it`s worth filing a bug to Clang analyzer…

[UPD:]

However, there is one case where prev might point to an uninitialized code element. This happens right after a table drop, so prev equals the table drop code, and ctbl equals the end-of-stream code. Both codes cannot be used as color codes, so the garbage contained there cannot be used to color anything.

But still, to be fully garbage-free, we just need to correct one line. Please change

    for (curr = ctbl++; curr; code[--curr] = 0); /** actual color codes **/

to

    for (curr = ++ctbl; curr; code[--curr] = 0); /** actual color codes **/

and see if that makes Clang analyzer happy.

hidefromkgb commented 5 years ago

If it does, please also apply the analyzer to the latest commit, 0ed5be0a2de7146dde6b0b7cb647ee8309aa86f5, and see if it finds anything.

wcout commented 5 years ago

It still gives the one same warning with the latest commit.

hidefromkgb commented 5 years ago

Well, OK. Does it also complain if the loop goes as follows?

    for (curr = ++ctbl; curr; )
        code[--curr] = 0;
wcout commented 5 years ago

Yes

hidefromkgb commented 5 years ago

Please show me the report.

wcout commented 5 years ago

report2.zip

hidefromkgb commented 5 years ago

Well, now that does seem like a bug in the analyzer. The loop condition is now obviously true.

hidefromkgb commented 5 years ago

May I close this ticket?

wcout commented 5 years ago

Yes please.

Your analysis seems profound, so it is really most probably a clang analyzer issue. (Though I hoped, that fixing the memory corruptions would make this warning go away too).