Summary: | Coverity fixes. | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Krzysztof Kowalczyk <kkowalczyk> | ||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | 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
Krzysztof Kowalczyk
2007-04-04 21:07:21 PDT
Created attachment 13956 [details]
coverity patch 00
Created attachment 13957 [details]
coverity patch 01
Created attachment 13958 [details]
coverity patch 02
Created attachment 13959 [details]
coverity patch 03
Created attachment 13960 [details]
coverity patch 04
Created attachment 13961 [details]
coverity patch 05
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.
Created attachment 13962 [details]
coverity patch 06
Created attachment 13963 [details]
coverity patch 07
Please don't post a huge # of unrelated patches in the same bug. If these are separate issues they should be separate bugs. Created attachment 13964 [details]
coverity patch 08
Created attachment 13965 [details]
coverity patch 09
Created attachment 13966 [details]
coverity patch 10
Created attachment 13967 [details]
coverity patch 11
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 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 on attachment 13967 [details]
coverity patch 11
How does *removing* a NULL-check improve things in this case? This looks incorrect to me.
(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. (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. (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 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 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 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 on attachment 13957 [details] coverity patch 01 landed in r20733 Comment on attachment 13960 [details]
coverity patch 04
landed in 20734
Comment on attachment 13961 [details]
coverity patch 05
landed in 20735
Comment on attachment 13963 [details]
coverity patch 07
landed in 20736
Comment on attachment 13964 [details] coverity patch 08 landed in r20737 Comment on attachment 13965 [details]
coverity patch 09
landed in 20738
Comment on attachment 13959 [details]
coverity patch 03
r=me
Comment on attachment 13959 [details] coverity patch 03 landed in r20775 Comment on attachment 13962 [details] coverity patch 06 landed in r20776 Comment on attachment 13967 [details] coverity patch 11 landed in r20777 |