Re: Code review needed ,spotted by Coverity

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Frederic Crozat wrote:
Le mercredi 12 avril 2006 à 01:44 -0400, Patrick Lam a écrit :

Frederic Crozat wrote:


-defect #759 in fccharset.c / FcCharSetSubtractCount :
*bm might be NULL because of assignment to bi.leaf->map and then it is
accessed without any NULL test. I don't know if bi.leaf->map is never
NULL.

I don't understand this code yet. The problem is not that ->map is NULL, but that bi might be NULL. ->map can't be null, it's a FcChar32[256/32].


Well, I didn't understand it either. But you are right about the
incorrect access, the defect is about bi being NULL.

I believe that it's safe, although I'm only 95% sure.

-defects #783, #784, #785, #786 : * if config->maxObjects == 0, but config->substPattern or
config->substFont are not NULL, st, while NULL, will be accessed
* at line 1497, there is a test against thisValue being NULL (so, it
might be NULL), but FcConfigDel called at line 1506 might deferences
thisValue, causing a crash.
* at line 1463, l might be leaked if switch (e->op) is handled by
default case). I don't know if it is possible.

Can you give more details on these defects?


Here are the url for the defects, they seems to work, I don't know for
how long. Otherwise, I'll copy the defect page elsewhere.

http://scan.coverity.com:7468/view-error.cgi?id=7529&runid=19&user=fcrozat&magic=636dfd390b4c5cc1f308d2841e3a3c5e&prevpage=query-run-table.cgi%3Ffile%3Dfontconfig%26magic%3D636dfd390b4c5cc1f308d2841e3a3c5e%26prevpage%3D%252Findex.cgi%253Fmagic%253D636dfd390b4c5cc1f308d2841e3a3c5e%2526user%253Dfcrozat%26runid%3D19%26user%3Dfcrozat&prevpagename=Search%20Results

The (hidden) invariant here is that config->maxObjects bounds from above the number of possible iterations that the for loop may take. If maxObjects is 0, the loop body is never executed, so st is never dereferenced.

http://scan.coverity.com:7468/view-error.cgi?id=10169&runid=21&user=fcrozat&magic=636dfd390b4c5cc1f308d2841e3a3c5e&prevpage=query-run-table.cgi%3Ffile%3Dfontconfig%26magic%3D636dfd390b4c5cc1f308d2841e3a3c5e%26prevpage%3D%252Findex.cgi%253Fmagic%253D636dfd390b4c5cc1f308d2841e3a3c5e%2526user%253Dfcrozat%26runid%3D21%26user%3Dfcrozat&prevpagename=Search%20Results

Looks plausible; I've put in a test for nullness.

http://scan.coverity.com:7468/view-error.cgi?id=10470&runid=21&user=fcrozat&magic=636dfd390b4c5cc1f308d2841e3a3c5e&prevpage=query-run-table.cgi%3Ffile%3Dfontconfig%26magic%3D636dfd390b4c5cc1f308d2841e3a3c5e%26prevpage%3D%252Findex.cgi%253Fmagic%253D636dfd390b4c5cc1f308d2841e3a3c5e%2526user%253Dfcrozat%26runid%3D21%26user%3Dfcrozat&prevpagename=Search%20Results

http://scan.coverity.com:7468/view-error.cgi?id=10469&runid=21&user=fcrozat&magic=636dfd390b4c5cc1f308d2841e3a3c5e&prevpage=query-run-table.cgi%3Ffile%3Dfontconfig%26magic%3D636dfd390b4c5cc1f308d2841e3a3c5e%26prevpage%3D%252Findex.cgi%253Fmagic%253D636dfd390b4c5cc1f308d2841e3a3c5e%2526user%253Dfcrozat%26runid%3D21%26user%3Dfcrozat&prevpagename=Search%20Results

Should be freed in the default case, which I've changed. That should fix both of these.

There are two new defects for some cases which were not completely fixed
with yesterday patches.

First one is in fcfreetype.c, some deadcode was still there. Based on
bedhad comment ("if you ask me, you should remove both goto Fail's in
that loop. it's trying to list scripts supported by the font, and if one
of the subtables is missing, we can simply ignore that script, instead
of failing the entire operation), I've cooked one patch which remove two
of the goto to fix defect #2088.

I'll take behdad's word for it...

The other defect is a memory leak in FcPatternFreeze (fcpat.c) in error
case (defect #2089).

Fixed; committing shortly.

pat
_______________________________________________
Fontconfig mailing list
Fontconfig@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/fontconfig

[Index of Archives]     [Fedora Fonts]     [Fedora Users]     [Fedora Cloud]     [Kernel]     [Fedora Packaging]     [Fedora Desktop]     [PAM]     [Gimp Graphics Editor]     [Yosemite News]

  Powered by Linux