Closed Bug 1273152 Opened 8 years ago Closed 8 years ago

[Static Analysis][Dereference after null check] In code generated by Codegen.py

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID
Tracking Status
firefox49 --- affected

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1361628, CID 1361627, CID 1361626, CID 1361624, CID 1361623, CID 1361622 btpp-active)

Attachments

(1 file)

the code generated by  Codegen.py in CGDictionary(CGThing) function can produce a cpp file that during execution can lead to a null pointer dereference even though it was previously null checked. As follows:

>>        if self.needToInitIds:
>>            body += fill(
>>                """
>>                ${dictName}Atoms* atomsCache = nullptr;
>>                if (cx) {
>>                  atomsCache = GetAtomCache<${dictName}Atoms>(cx);
>>                  if (!*reinterpret_cast<jsid**>(atomsCache) && !InitIds(cx, atomsCache)) {
>>                    return false;
>>                  }
>>                }
>>
>>                """,
>>                dictName=self.makeClassName(self.dictionary))
>>
>>        if self.dictionary.parent:
>>            body += fill(
>>                """
>>                // Per spec, we init the parent's members first
>>                if (!${dictName}::Init(cx, val)) {
>>                  return false;
>>                }
>>
>>                """,
>>                dictName=self.makeClassName(self.dictionary.parent))
>>        else:
>>            body += dedent(
>>                """
>>                { // scope for isConvertible
>>                  bool isConvertible;
>>                  if (!IsConvertibleToDictionary(cx, val, &isConvertible)) {
>>                    return false;
>>                  }
>>                  if (!isConvertible) {
>>                    return ThrowErrorMessage(cx, MSG_NOT_DICTIONARY, sourceDescription);
>>                  }
>>                }
>>
>>                """)

As an example we can take function MediaKeySystemConfiguration::Init from MediaKeySystemAccessBinding.cpp


>>  if (cx) {
>>    atomsCache = GetAtomCache<MediaKeySystemConfigurationAtoms>(cx);
>>    if (!*reinterpret_cast<jsid**>(atomsCache) && !InitIds(cx, atomsCache)) {
>>      return false;
>>    }
>>  }
>>
>>  { // scope for isConvertible
>>    bool isConvertible;
>>    if (!IsConvertibleToDictionary(cx, val, &isConvertible)) {
>>      return false;
>>    }
>>    if (!isConvertible) { 	
>>      return ThrowErrorMessage(cx, MSG_NOT_DICTIONARY, sourceDescription);
>>    }
>>  }

I think we should modify the py file in order to prevent null dereferences.
Comment on attachment 8752853 [details]
MozReview Request: Bug 1273152 - modify autogen python to prevent null pointer dereference on generates cpps. r?jst

Redirecting to peterv. We're at the very least inconsistent in where we check for cx being null in the generated code... One question I have is whether we should just check once rather than checking separately in the 3 places where I saw the checks being done in a single sample of actual generated code. Regardless, peterv knows this far better than I do.
Attachment #8752853 - Flags: review?(jst) → review?(peterv)
Whiteboard: CID 1361628, CID 1361627, CID 1361626, CID 1361624, CID 1361623, CID 1361622 → CID 1361628, CID 1361627, CID 1361626, CID 1361624, CID 1361623, CID 1361622 btpp-active
Comment on attachment 8752853 [details]
MozReview Request: Bug 1273152 - modify autogen python to prevent null pointer dereference on generates cpps. r?jst

The code is fine as is, it even has comments about a null cx being ok if we're initing from null.
Attachment #8752853 - Flags: review?(peterv) → review-
To expand on that, IsConvertibleToDictionary doesn't use cx if val is a null JS::Value.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: