Closed Bug 425454 Opened 12 years ago Closed 12 years ago

Need a way to whitelist/ignore correct uses of placement-new for NS_STACK_CLASS

Categories

(Firefox Build System :: Source Code Analysis, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benjamin, Assigned: benjamin)

Details

Attachments

(3 files, 1 obsolete file)

I'm trying to set NS_STACK_CLASS on nsAutoString, using the following patch against mozilla-central: http://hg.mozilla.org/users/bsmedberg_mozilla.com/mozilla-central-patches/?file/7e29abf95241/stack-autostring

This works until the tricky code here:

http://mxr.mozilla.org/mozilla/source/js/src/xpconnect/src/xpcwrappednative.cpp#2148

This code allocates a stack buffer for the autostring but doesn't actually create a new string in that buffer unless it needs to, using placement-new. I'd like a way to annotate that this particular pattern is ok (normally calling "new" on a NS_STACK_CLASS is disallowed).

I don't think I want to allow all calls to placement-new.
Are you saying (1) you want to use an attribute to manually annotate certain allocation sites as being OK, or (2) an analysis that detects this pattern so it can be filtered out of your checker?

Do I have it right that what makes this OK is that (a) the allocation uses placement new (on address A) and (b) A points to stack space in the current frame? I think it wouldn't be hard to detect at least easy cases of this property, such as the one in your example. It probably needs Treehydra, but maybe Dehydra could do it. Taras probably has a clearer idea on that.
Not sure what I want, really. I don't see an annotation that would make this easier, really: we already know that "autoString" is a local (stack) variable: I guess maybe what I want is a special rule for static-checking.js of the form "you can call ::operator new(void *loc) for a NS_STACK_CLASS if "loc" is the address of a local variable."

Let me try a couple things and report back.
From further investigation: currently I don't actually do the blocking based on the call to operator new, because the call to operator new doesn't have type information about what's being constructed. Instead, I check the .fieldOf on calls to constructors: if the constructor .fieldOf is a pointer, then it's a "new A()" otherwise it's an auto (stack) A.

So any analysis here would need some way to associate the call to "new" with the subsequent type that was being created.
Attached file Testcase
Testcase file... you can probably remove the nscore.h dependency by inlining __attribute__((user("NS_stack")))
Attached file Dehydra scriptage
Here's a Dehydra script that *may* do enough to solve your problem. In process_function, I run a pass that for every call to operator new() with an associated ctor, adds a .destCtor to the Dehydra object for operator new(). I.e., you get a link from the new to the ctor. Should be easy to create other links if you need them.

For placement new, it looks like Dehydra currently gives the signature, so you can tell it is placement new, and the args, so you can check if the memory is from a stack var. If I'm missing a piece here and you need extra info from Dehydra, let me know.

The attached script has a lot of library code at the top. Look for "BUG 425454" to find the bug-specific code. find_heap_ctor has the actual logic. The tricky bit is that you have to call it with a "dehydra group" (e.g., the array representing the rhs of an assignment): I do this via body_groups. If you run this script as-is, it should print out all the new/ctor pairs it finds in the code.
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
(In reply to comment #6)
> Created an attachment (id=327165) [details]
> Whitelist placement-new for stack classes, rev. 1

Looks pretty good. I didn't know about IDENTIFER_OPNAME_P. That's useful. A few remarks:

- tmap looks it's not really being used, along with the loop over 'stack'. Or maybe I'm not reading it right.

- comment on operator_new_assign is cryptic to the new reader.

- it is in no way important, but FYR things like:

      let destType = dehydra_convert(TREE_TYPE(destVar));
      if (!destType.isPointer && !destType.isReference)

can be replaced by

      let destTypeCode = TREE_CODE(TREE_TYPE(destVar));
      if (destTypeCode != POINTER_TYPE && destTypeCode != REFERENCE_TYPE)

> - tmap looks it's not really being used, along with the loop over 'stack'. Or
> maybe I'm not reading it right.

Well... I thought tmap was a requirement of the walk_tree API. It's only used to filter elements so you don't process them twice.

As for the loop over stack, I'm pretty sure it is used: some trees don't have locations, so we walk up the stack looking for the closest node that does have a location, and use that.
Hopefully make the comment more clear. I moved IDENTIFIER_OPNAME_P to gcc_compat.js
Attachment #327165 - Attachment is obsolete: true
Attachment #327405 - Flags: review?(tglek)
(In reply to comment #8)
> > - tmap looks it's not really being used, along with the loop over 'stack'. Or
> > maybe I'm not reading it right.
> 
> Well... I thought tmap was a requirement of the walk_tree API. It's only used
> to filter elements so you don't process them twice.

It's not a requirement of the API, but it does do what you said. Basically it means that it will not visit decl nodes(which are the main things that repeat) twice which may or may not be what you want.

Comment on attachment 327405 [details] [diff] [review]
Whitelist placement-new for stack classes, rev. 1.1

>+
>+function iter_statement_list(sl)
>+{
>+  for (let ptr = STATEMENT_LIST_HEAD(sl);
>+       ptr;
>+       ptr = ptr.next)
>+    yield ptr.stmt;
>+}

this should probably go into gcc_util.js

Otherwise looks ok to me.
Attachment #327405 - Flags: review?(tglek) → review+
Moved iter_statement_list to gcc_util.js and pushed this to mozilla-central. Revision 265eb7adf867
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 327405 [details] [diff] [review]
Whitelist placement-new for stack classes, rev. 1.1

>+function process_type(c)
>+{
>+  if ((c.kind == 'class' || c.kind == 'struct') && !c.isIncomplete) {
>+    for each (let base in c.bases)
>+      if (isFinal(base))
>+	error("Class '" + c.name + "' derives from final class '" + base.name + "'.", c.loc);

Drive-by indentation missing nit.

>+    for each (let base in c.bases)
>+    if (isStack(base))
>+      return true;

Again (both lines in the for each).

>+    for each (let member in c.members) {
>+      if (member.isFunction)
>+	continue;

Ditto.

>+
>+      let type = member.type;
>+      while (true) {
>+	if (type === undefined)
>+	  break;
>+
>+	if (type.isArray) {
>+	  type = type.type;
>+	  continue;
>+	}
>+
>+	if (type.typedef) {
>+	  if (hasAttribute(type, 'NS_stack'))
>+	    return true;
>+
>+	  type = type.typedef;
>+	  continue;
>+	}
>+	break;
>+      }

The while (true) body is underindented too.

Are there hard tabs in the file?

/be
Pushed a fix for the whitespace issues, which were hard tabs (and set global indent-tabs-mode: nil to avoid in the future)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.