Bug 13285

Summary: Coverity fixes.
Product: WebKit Reporter: Krzysztof Kowalczyk <kkowalczyk>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: mrowe
Priority: P2    
Version: 523.x (Safari 3)   
Hardware: PC   
OS: OS X 10.4   
Attachments:
Description Flags
coverity patch 00
hyatt: review-
coverity patch 01
darin: review+
coverity patch 02
mrowe: review-
coverity patch 03
darin: review+
coverity patch 04
darin: review+
coverity patch 05
rwlbuis: review+
coverity patch 06
darin: review+
coverity patch 07
darin: review+
coverity patch 08
darin: review+
coverity patch 09
darin: review+
coverity patch 10
darin: review-
coverity patch 11 darin: review+

Description Krzysztof Kowalczyk 2007-04-04 21:07:21 PDT
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.
Comment 1 Krzysztof Kowalczyk 2007-04-04 21:09:56 PDT
Created attachment 13956 [details]
coverity patch 00
Comment 2 Krzysztof Kowalczyk 2007-04-04 21:10:24 PDT
Created attachment 13957 [details]
coverity patch 01
Comment 3 Krzysztof Kowalczyk 2007-04-04 21:10:48 PDT
Created attachment 13958 [details]
coverity patch 02
Comment 4 Krzysztof Kowalczyk 2007-04-04 21:11:08 PDT
Created attachment 13959 [details]
coverity patch 03
Comment 5 Krzysztof Kowalczyk 2007-04-04 21:11:27 PDT
Created attachment 13960 [details]
coverity patch 04
Comment 6 Krzysztof Kowalczyk 2007-04-04 21:11:50 PDT
Created attachment 13961 [details]
coverity patch 05
Comment 7 Dave Hyatt 2007-04-04 21:12:00 PDT
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.
Comment 8 Krzysztof Kowalczyk 2007-04-04 21:12:11 PDT
Created attachment 13962 [details]
coverity patch 06
Comment 9 Krzysztof Kowalczyk 2007-04-04 21:12:30 PDT
Created attachment 13963 [details]
coverity patch 07
Comment 10 Dave Hyatt 2007-04-04 21:12:44 PDT
Please don't post a huge # of unrelated patches in the same bug.  If these are separate issues they should be separate bugs.
Comment 11 Krzysztof Kowalczyk 2007-04-04 21:12:52 PDT
Created attachment 13964 [details]
coverity patch 08
Comment 12 Krzysztof Kowalczyk 2007-04-04 21:13:12 PDT
Created attachment 13965 [details]
coverity patch 09
Comment 13 Krzysztof Kowalczyk 2007-04-04 21:13:28 PDT
Created attachment 13966 [details]
coverity patch 10
Comment 14 Krzysztof Kowalczyk 2007-04-04 21:13:48 PDT
Created attachment 13967 [details]
coverity patch 11
Comment 15 Mark Rowe (bdash) 2007-04-04 21:45:12 PDT
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.
Comment 16 Mark Rowe (bdash) 2007-04-04 21:49:47 PDT
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?
Comment 17 Mark Rowe (bdash) 2007-04-04 21:55:07 PDT
Comment on attachment 13967 [details]
coverity patch 11

How does *removing* a NULL-check improve things in this case?  This looks incorrect to me.
Comment 18 Krzysztof Kowalczyk 2007-04-04 23:47:13 PDT
(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.
Comment 19 Krzysztof Kowalczyk 2007-04-04 23:52:52 PDT
(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.


Comment 20 Krzysztof Kowalczyk 2007-04-04 23:59:04 PDT
(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.

Comment 21 Darin Adler 2007-04-05 08:34:09 PDT
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.
Comment 22 Darin Adler 2007-04-05 08:37:45 PDT
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.
Comment 23 Rob Buis 2007-04-05 13:16:32 PDT
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.
Comment 24 Krzysztof Kowalczyk 2007-04-05 15:51:35 PDT
Comment on attachment 13957 [details]
coverity patch 01

landed in r20733
Comment 25 Krzysztof Kowalczyk 2007-04-05 15:55:44 PDT
Comment on attachment 13960 [details]
coverity patch 04

landed in 20734
Comment 26 Krzysztof Kowalczyk 2007-04-05 16:00:04 PDT
Comment on attachment 13961 [details]
coverity patch 05

landed in 20735
Comment 27 Krzysztof Kowalczyk 2007-04-05 16:02:41 PDT
Comment on attachment 13963 [details]
coverity patch 07

landed in 20736
Comment 28 Krzysztof Kowalczyk 2007-04-05 16:07:20 PDT
Comment on attachment 13964 [details]
coverity patch 08

landed in r20737
Comment 29 Krzysztof Kowalczyk 2007-04-05 16:10:20 PDT
Comment on attachment 13965 [details]
coverity patch 09

landed in 20738
Comment 30 Darin Adler 2007-04-06 13:44:01 PDT
Comment on attachment 13959 [details]
coverity patch 03

r=me
Comment 31 Krzysztof Kowalczyk 2007-04-06 20:19:44 PDT
Comment on attachment 13959 [details]
coverity patch 03

landed in r20775
Comment 32 Krzysztof Kowalczyk 2007-04-06 20:23:08 PDT
Comment on attachment 13962 [details]
coverity patch 06

landed in r20776
Comment 33 Krzysztof Kowalczyk 2007-04-06 20:25:31 PDT
Comment on attachment 13967 [details]
coverity patch 11

landed in r20777