[xmlsec] Patches for issues found by automated check tools

[Simo Sorce]

On Wed, 2014-06-04 at 10:05 -0700, Aleksey Sanin wrote:
> Hi Simo,
> 
> Thanks again for the patches. I've applied most of them (see bellow) or
> just fixed problems in other ways. The code is pushed to Gnome git and
> I wonder if it would be possible to re-run the tools to make sure no
> new issues are found? Thanks in advance!
> 
> 
> * 0001-Check-returned-value.patch
> 
> Looks good, commited

Not that I care much, but why did you change the commit author ?
Other projects leave credit in git commits ...

> * 0002-Fix-dead-code.patch
> 
> I don't believe the first part of the patch is correct: assert will
> fire if condition is "false" which is what the original code does:
> 
> xmlSecAssert2("..." == NULL, 1)
> 
> Since string != NULL, it will fire (ugly, I know). What it actually
> complains about is the return(-1) *after* that - assert always fires
> and returns so second return is never reached. So to fix this one, I've
> replaced assert with a real error.

Your fix is clearer indeed.

> The second part - Coverity is correct with the useVisa3DHack flag but
> I feel much safer having the dead code that tries to free buffer instead
> of not having it.

Maybe just set a return error variable and "goto done;" ?

That way further modification won't risk forgetting and you avoid
unnecessary duplication of boilerplate.

> * 0003-Fix-potential-NULL-pointer-dereference.patch
> 
> Looks good, applied with a minor tweak.
> 
> * 0004-Fix-printf-format-warnings.patch
> 
> I hate printf() integer formats. Applied.
> 
> * 0005-Silence-warnings-about-unused-computed-values.patch
> 
> Don't think it matters much. Applied.
> 
> * 0006-Elimanate-assignments-that-are-never-used.patch
> 
> The keysdata.c patch is wrong - this is actually a good catch and ret
> should be checked.

Yeah I wasn't sure about this, but I figured you'd know :-)

> The pkikeys.c patch is also wrong - a check is needed.

Ack.

> The nss/x509vfy.c patch is badly wrong - it breaks the parser. I've set
> the initial value instead.

Not sure why you say this, the error was not missing initialization, but
that valueLen set in the else branch is never used.
Looking at the code valueLen all the time you set valueLen before using
it. It is never used untouched after you set it in the else branch,
hence why the checker correclty marked it as dead code. My manual
inspection seem to confirm it, but maybe there is some macro that uses
it I am not seeing ?
Initializing it is also useless because you never use that value unless
you first initialized it with the return of
xmlSecNssX509NAmeStringRead() in one of the 2 if branches.

> The src/openssl/x509vfy.c is partially correct. The flag variable is
> important and is actually used (I guess Clang got confused by all the
> #if's in this file).

ok.

> The parser.c patch is wrong too - again good catch and ret should be
> checked.

ack

> Anyway, the errors were good - I just patched them in different ways.
> 
> * 0007-Fix-potential-NULL-dereference.patch
> 
> Looks good, applied with a minor tweak.

I was able to make only a coverity run on master and all errors for
which I sent patches are gone (I have a couple false positives I didn't
bother with), except the dead_code warnings about free(buf) as expected.

I will try to do the other scans in the next few days.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York