RDFContentSinkImpl::GetIdAboutAttribute can return an unintialized value

RESOLVED FIXED in mozilla1.8.1beta1

Status

()

Core
RDF
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: timeless, Assigned: Gavin)

Tracking

({coverity, fixed1.8.1})

1.8 Branch
mozilla1.8.1beta1
coverity, fixed1.8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

12 years ago
 

Comment 1

12 years ago
Timeless, a bit more care when filing these bugs would be good, the URL you gave
is rather pointless.
I can only assume that this bug is about rv not being initialized in all cases?
Or is aResource, too?
Coverity complains about returning an unitialized rv. It's CID 482 at scan.coverity.com:7454, if you want to get an account. If 908 is hit, and 928 isn't, then rv is returned, unitialized.

Comment 3

12 years ago
If you reset that dumb URL again, I'll just INVALID this bug.

That URL highlights stuff that is commented out PR_LOG foo. It's useless.
(Reporter)

Comment 4

12 years ago
the prlog is in the block of code that would be executed along that path. normally i'd highlight the closing brace, but that's hardly more intuitive.

Comment 5

12 years ago
RDF is not the right component to file bugs on coverty problems, and this bug
seems to be about that URL only, not about getting potential problems described,
and eventually fixed.

INVALID, 'cause I'm not joking.

If you want that issue resolved, file a bug with descent information.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → INVALID
(Reporter)

Comment 6

12 years ago
this bug is clearly limited to rdf. it belongs in the rdf component. if i fixed this code behind your back, you'd complain.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Status: REOPENED → NEW
OS: Linux → All
Hardware: PC → All

Comment 7

12 years ago
If you'd fix the problem based on the information in that URL, I'd be highly
suprised. Thus, INVALID.
Status: NEW → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → INVALID
This is not invalid. I described the problem, and the URL, in comment #2. What isn't clear?
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
So the point is that if nodeID is nonempty and the Get() gives us something, we'll return a random rv.  That totally looks like a bug to me, unless there something that guarantees that this can't happen.  If there is something like that, I don't see it (and if there is, this code should assert instead of null-checking at line 927).

Note that I don't care what the URI in the URI field highlights; the combination of the coverity report and the code in question is very clear -- RDF has a bug here.
Severity: normal → major
Flags: blocking1.9a2?
OS: All → Linux
Hardware: All → PC
And pike, if you want to clear the URI field I again don't care.  But we do need to fix the actual bug.

Comment 11

12 years ago
Just check the code with aAttributes == nsnull.
I offered to remove the bad information provided by the URL, which was not
accepted.
The behaviour described by the URL is not the problem.

If you want a bug on it, file a new one, this one is full of crap.
Severity: major → normal
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
OS: Linux → All
Hardware: PC → All
Resolution: --- → INVALID
> Just check the code with aAttributes == nsnull.

Why does that matter?  That doesn't fix the bogus case.

If you want a better URL, try:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/rdf/base/src/nsRDFContentSink.cpp&rev=1.127&mark=907,924,927#900

If the first two checks succeed and the third one gives false, we will return a random number.

Reopening again.  I see absolutely no reason for a bug with a clear problem description to be marked invalid.  If you do, please feel free to explain it to me over e-mail or IRC, but you'll have to be damned convincing. Note that the fact that bonsai markings can't be annotated is NOT a good reason to invalid a bug if the annotations are being provided out-of-band in the bug.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Created attachment 218610 [details] [diff] [review]
patch

I think this is correct: in all other cases, rv is assigned to before return, and if (!*aResource) is false, then we've succeeded. GetAnonymousResource (line 928) also returns NS_OK on success, so this should be what is expected, and should only change the buggy case.
Assignee: nobody → gavin.sharp
Status: REOPENED → ASSIGNED
Attachment #218610 - Flags: superreview?(bzbarsky)
Attachment #218610 - Flags: review?(axel)
Though maybe this function should be refactored to make what's happening clearer, if any of you have any other suggestions.
Comment on attachment 218610 [details] [diff] [review]
patch

Looks reasonable.
Attachment #218610 - Flags: superreview?(bzbarsky) → superreview+

Updated

12 years ago
Attachment #218610 - Flags: review?(axel) → review+
mozilla/rdf/base/src/nsRDFContentSink.cpp 	1.128
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago12 years ago
Flags: blocking1.9a2?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
Attachment #218610 - Flags: approval-branch-1.8.1?(axel)

Updated

12 years ago
Attachment #218610 - Flags: approval-branch-1.8.1?(axel) → approval-branch-1.8.1+
Whiteboard: [checkin needed (1.8 branch)]
mozilla/rdf/base/src/nsRDFContentSink.cpp 	1.121.8.3
Keywords: fixed1.8.1
Whiteboard: [checkin needed (1.8 branch)]
Target Milestone: mozilla1.9alpha → mozilla1.8.1beta1
Version: Trunk → 1.8 Branch
You need to log in before you can comment on or make changes to this bug.