[Bf-committers] bugs?

Campbell Barton ideasman42 at gmail.com
Sat Feb 12 03:37:48 CET 2011


You suggest we should eventually make blender warning free, I'd like
this too but at the moment there are a enough false positives mixed in
with Clang's warnings.
Or, at least warnings which will never happen unless blender runs into
memory corruptions/un-thread-safe code.
In practice this isn't so simple.

Examples:
Clang's static checker considers that checks on a value which is a
pointer passed to the argument may not be the same twice: Example, see
checks on (mface[i].flag & ME_SMOOTH)
http://www.graphicall.org/ftp/ideasman42/2011-01-07-2/report-dOTWay.html#EndPath

In this case it can be silenced by assigning a const and checking that instead:
   const int smoothnormal = (mface[i].flag & ME_SMOOTH);
    if(smoothnormal)...
    ...
	if(smoothnormal)...

... you can see its unreasonable to assign const's everywhere just
because a variable could theoretically be changed by another thread
and cause a check to be true once and false the next.

Clang also gives false positives for a lot of python C/API usage,
creating a tuple and assigning variables to it for eg.


Regarding string lengths.
I didn't mean to bring this up since blender's code has lots of
potential buffer overruns, every so often I try to reduce these by
using BLI_snprintf and similar but we still have enough unchecked uses
of sprintf, strcpy().
Of course these _should_ be changed, on the other hand there are lots
of bugs in the tracker and hardly any are to do with buffer overruns
because over time we fixed reported cases.

Since you point this out, I changes the function so the values are
printed directly then the stdout flushed. r34785.
This is easier then imposing string lengths.


As for the original issue, I disagree that  dead
assignments/increments for the last assigned value should be removed.
>From this example you can see how removing the dead code wouldn't be
obvious to add back later if further calls were added.
http://www.graphicall.org/ftp/ideasman42/2011-01-07-2/report-2ZcLF2.html#EndPath

Using (void)value; isn't that strange, Nokia's QT-Toolkit uses a macro
for this to silence unused argument warnings.
#define Q_UNUSED(x) (void)x;

We could #ifdef a macro like this for MSVC is needed.
Not suggesting we add this but I'd prefer this to removing assignments
potentially adding confusion later.

On Fri, Feb 11, 2011 at 5:41 PM, Lars Krueger <lars_e_krueger at gmx.de> wrote:
>
> -------- Original-Nachricht --------
>> Datum: Fri, 11 Feb 2011 05:52:54 +0000
>> Von: Campbell Barton <ideasman42 at gmail.com>
>> An: bf-blender developers <bf-committers at blender.org>
>> Betreff: Re: [Bf-committers] bugs?
>
>> The reason I didn't continue to clear the dead assignment warnings is
>> it would do more harm then good IMHO.
>
> Shouldn't warning free code be the goal here, at least over time? In the following article they ran a checker on the linux kernel and looked for redundant code. Their checker is more strict than a C compiler, but after I read this paper I started to pay more attention to warnings.
>
> Link: theory.stanford.edu/~yxie/tse-redundancy.pdf
>
> A quote from the paper: "Reassigning values is typically harmless, but it does signal fairly confused programmers."
>
> The code below is actually a case in point that these warnings indicate potential problems, see below. My points can be seen as nitpicking, but try to see them in the light of the quote above.
>
>> Example
>> ----------
>> static void stats_background(void *UNUSED(arg), RenderStats *rs)
>> {
>>     char str[400], *spos= str;
>
> Can we prove that the string will never be longer than 399 characters?
>
>>     spos+= sprintf(spos, "Fra:%d Mem:%.2fM (%.2fM, peak %.2fM) ",
>> rs->cfra,megs_used_memory, mmap_used_memory, megs_peak_memory);
>
> This should check if we WOULD write beyond the end of str BEFORE sprintf is actually called. There is no use to check AFTER sprintf was run as this might have overwritten the stack already. It's a crash or buffer-overflow attack in waiting.
>
>>     if(rs->infostr) {
>>         spos+= sprintf(spos, "| %s", rs->infostr);
>>     }
>>     else {
>>         if(rs->tothalo)
>>             spos+= sprintf(spos, "Sce: %s Ve:%d Fa:%d Ha:%d La:%d",
>> rs->scenename, rs->totvert, rs->totface, rs->tothalo, rs->totlamp);
>>         else
>>             spos+= sprintf(spos, "Sce: %s Ve:%d Fa:%d La:%d",
>> rs->scenename, rs->totvert, rs->totface, rs->totlamp);
>>     }
>>     printf("%s\n", str);
>> }
>> ----------
>>
>> You can tell that the last 3 'spos +=' are dead assignments, but if
>> these are removed, a developer might add a new string later and forget
>> to add one of them back.
>
> Which could be prevented by better design that also checks for the proper length. My suggestion:
>
> size_t offs=0;
> char str[400];
>
> #define APPEND1( frmt, param) { int rc=snprintf( str+offs, sizeof(str)/sizeof(str[0])-1-offs, frmt, param); offs+=(rc<0)?0:rc; }
>
>    if(rs->infostr)
>       APPEND1("| %s", rs->infostr)
>
> Code like this would remove the warning (both str and offs are used) and the buffer overflow problem.
>
> The APPEND1 macro could be replaced by a variadic function or (assuming at least C99) a variadic macro.
>
>> Same goes for 'yofs += ' with many interface functions.
>
> Leave out the last and you're done. If a programmer forgets to add it, it's easy to see.
>
>> In some cases I commented the assignment, eg:
>>         /*spos+=*/ sprintf(spos, "| %s", rs->infostr);
>
> This cures the symptom, but not the disease.
>
>> But this is only good if the variable its self is commented otherwise
>> it could be used later where the developer needs to remember to
>> uncomment the assignment which isn't always obvious.
>
> Well, no. The macro design above is fairly self-documenting where it is used. I would a sentence or two at the definition.
>
>> To quiet Clang's dead assignment warnings we could do this at the end
>> of functions...
>> ----------
>>     (void)spos;
>
> Shudder. Aside from increasing the WTF/minute, I think this triggers "dead code" warnings with /W4 in Visual Studio.
>
>> It would be nice to quiet the warnings so we know all warnings are
>> _real_ problems and real bugs don't get mixed up with harmless reports
>> but this also adds cruft which is why I didn't go ahead with it.
>
> Most warnings are an indication that something is wrong.
>
>> If other devs think its worth trying to get even fewer warnings we
>> could in the same way its nice to have 0 compiler warnings, but at
>> this point I feel we're getting diminishing returns for making such
>> small changes.
>
> Well, that mostly depends on how many other 399 character buffers there are and which stack frame they overwrite on overflow.
> --
> Dr. Lars Krueger
>
>
> Empfehlen Sie GMX DSL Ihren Freunden und Bekannten und wir
> belohnen Sie mit bis zu 50,- Euro! https://freundschaftswerbung.gmx.de
> _______________________________________________
> Bf-committers mailing list
> Bf-committers at blender.org
> http://lists.blender.org/mailman/listinfo/bf-committers
>



-- 
- Campbell


More information about the Bf-committers mailing list