Bug 13285 – Coverity fixes.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Krzysztof Kowalczyk]

Coverity is a static analysis tool for C/C++ code. I've ran coverity on webkit code and this bug contains patches for the issues it found.

I created multiple patches (as opposed to one with all the fixes) because:
a) each issue is independent from others
b) they can be accepted/rejected independently

Not all of the issues reported by coverity are bugs that have obvious repro scenario but they're still worth fixing for the same reason it's worth fixing compiler warnings: when running coverity in the future, there will be less noise in the output hence real bugs will stand out.

[Krzysztof Kowalczyk]

Created attachment 13956 [details]
coverity patch 00

[Krzysztof Kowalczyk]

Created attachment 13957 [details]
coverity patch 01

[Krzysztof Kowalczyk]

Created attachment 13958 [details]
coverity patch 02

[Krzysztof Kowalczyk]

Created attachment 13959 [details]
coverity patch 03

[Krzysztof Kowalczyk]

Created attachment 13960 [details]
coverity patch 04

[Krzysztof Kowalczyk]

Created attachment 13961 [details]
coverity patch 05

[Dave Hyatt]

Comment on attachment 13956 [details]
coverity patch 00

This seems like it could cause an actual behavioral change.  Without a test or a changelog, I have to minus this.

[Krzysztof Kowalczyk]

Created attachment 13962 [details]
coverity patch 06

[Krzysztof Kowalczyk]

Created attachment 13963 [details]
coverity patch 07

[Dave Hyatt]

Please don't post a huge # of unrelated patches in the same bug.  If these are separate issues they should be separate bugs.

[Krzysztof Kowalczyk]

Created attachment 13964 [details]
coverity patch 08

[Krzysztof Kowalczyk]

Created attachment 13965 [details]
coverity patch 09

[Krzysztof Kowalczyk]

Created attachment 13966 [details]
coverity patch 10

[Krzysztof Kowalczyk]

Created attachment 13967 [details]
coverity patch 11

[Mark Rowe (bdash)]

Comment on attachment 13958 [details]
coverity patch 02

This "fix" looks to address a non-existent problem.  If data is NULL then newSize is set to 0 and newSize * d->itemSize will also be zero, so data will not be dereferenced.

[Mark Rowe (bdash)]

Comment on attachment 13959 [details]
coverity patch 03

Under what conditions will sourceRanges be NULL at the call to spliceSubstringsWithSeparators?  A quick reading of the code suggests to me that pushSourceRange will always be called at least once, which will result in sourceRanges being initialized.  When is this not the case?

[Mark Rowe (bdash)]

Comment on attachment 13967 [details]
coverity patch 11

How does *removing* a NULL-check improve things in this case?  This looks incorrect to me.

[Krzysztof Kowalczyk]

(In reply to comment #15)
> (From update of attachment 13958 [details] [edit])
> This "fix" looks to address a non-existent problem.  If data is NULL then
> newSize is set to 0 and newSize * d->itemSize will also be zero, so data
> will�not be dereferenced.

Like I explained in the beginning, even if a given problem is flagged by coverity because it cannot prove that memcpy() won't reference the possibly NULL pointer, it's still worth fixing so that when running coverity in the future won't show the same issue over and over again. It's the same logic you apply when fixing compiler warnings even if you know that ultimately they're benign.

[Krzysztof Kowalczyk]

(In reply to comment #16)
> (From update of attachment 13959 [details] [edit])
> Under what conditions will sourceRanges be NULL at the call to
> spliceSubstringsWithSeparators?  A quick reading of the code suggests to me
> that pushSourceRange will always be called at least once, which will result in
> sourceRanges being initialized.  When is this not the case?
when
      UString matchString = regExpObj->performMatch(reg, source, startPosition, &matchIndex, &ovector);

returns -1 and source.size() is 0. Can it really happen in practice? I don't know - the code is too complex for me to say either way with 100% certitude. Coverity seems to think it's possible and I can't prove it wrong.

[Krzysztof Kowalczyk]

(In reply to comment #17)
> (From update of attachment 13967 [details] [edit])
> How does *removing* a NULL-check improve things in this case?  This looks
> incorrect to me.

Coverity complains because this NULL check is, effectively, a no-op. Coverity sees a check for info_ptr != NULL so it infers that the programmer thinks that info_ptr can be NULL. But then it sees that info_ptr is blindly dereferenced anyway. So either the NULL check is wrong or the function will crash anyway in which case removing the check makes no difference (the function will crash in a different place).

This is not to say that adding more NULL checks wouldn't be a valid solution - it's just more invasive change and I was afraid of introducing changes in behaviour.

[Darin Adler]

Comment on attachment 13963 [details]
coverity patch 07

Just a test tool. If it wasn't I'd suggest reordering the buffer allocation code and the fopen rather than adding the free. But as it is, r=me.

[Darin Adler]

Comment on attachment 13966 [details]
coverity patch 10

One of the observers could call clear(), and thus m_image could be 0, so Coverity is wrong here.

[Rob Buis]

Comment on attachment 13961 [details]
coverity patch 05

Looks fine to me. I think a few lines above this one a similar code construct is there, not sure if you have a patch for that already, but the same fix should be applied AFAICS.

[Krzysztof Kowalczyk]

Comment on attachment 13957 [details]
coverity patch 01

landed in r20733

[Krzysztof Kowalczyk]

Comment on attachment 13960 [details]
coverity patch 04

landed in 20734

[Krzysztof Kowalczyk]

Comment on attachment 13961 [details]
coverity patch 05

landed in 20735

[Krzysztof Kowalczyk]

Comment on attachment 13963 [details]
coverity patch 07

landed in 20736

[Krzysztof Kowalczyk]

Comment on attachment 13964 [details]
coverity patch 08

landed in r20737

[Krzysztof Kowalczyk]

Comment on attachment 13965 [details]
coverity patch 09

landed in 20738

[Darin Adler]

Comment on attachment 13959 [details]
coverity patch 03

r=me

[Krzysztof Kowalczyk]

Comment on attachment 13959 [details]
coverity patch 03

landed in r20775

[Krzysztof Kowalczyk]

Comment on attachment 13962 [details]
coverity patch 06

landed in r20776

[Krzysztof Kowalczyk]

Comment on attachment 13967 [details]
coverity patch 11

landed in r20777