334207 - RDFContentSinkImpl::GetIdAboutAttribute can return an unintialized value

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect)

Description

[timeless]

Comment 1

[Axel Hecht]

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?

Comment 2

[:Gavin Sharp [email: [e-mail]]]

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

[Axel Hecht]

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.

Comment 4

[timeless]

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

[Axel Hecht]

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.

Comment 6

[timeless]

this bug is clearly limited to rdf. it belongs in the rdf component. if i fixed this code behind your back, you'd complain.

Comment 7

[Axel Hecht]

If you'd fix the problem based on the information in that URL, I'd be highly
suprised. Thus, INVALID.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

This is not invalid. I described the problem, and the URL, in comment #2. What isn't clear?

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

[Axel Hecht]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Attachment: 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.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Though maybe this function should be refactored to make what's happening clearer, if any of you have any other suggestions.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 218610 [details] [diff] [review]
patch

Looks reasonable.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

mozilla/rdf/base/src/nsRDFContentSink.cpp 	1.128

Comment 17

[:Gavin Sharp [email: [e-mail]]]

mozilla/rdf/base/src/nsRDFContentSink.cpp 	1.121.8.3