[Bf-committers] bugs?

[Lars Krueger]

-------- 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