431832 - Allow annotated outparams for methods returning PRBool or void

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Benjamin Smedberg]

Attachment: Annotate nsInterfaceHashtable

Some methods with outparams don't return nsresult: they return PRBool where PR_TRUE is success and PR_FALSE is failure. This is causing outparam errors in nsHashPropertyBag:

../../../src/xpcom/ds/nsHashPropertyBag.cpp:319: warning: outparam not written on NS_SUCCEEDED(return value): _retval
    Outparam declared at: ../../../src/xpcom/ds/nsHashPropertyBag.cpp:107:75
    Return at: ../../../src/xpcom/ds/nsHashPropertyBag.cpp:113:12

http://hg.mozilla.org/mozilla-central/index.cgi/file/f337857f6837/xpcom/ds/nsHashPropertyBag.cpp#l109

mPropertyHash.Get should be treated as a getter (and should also be analyzed for correctness) with the annotations I'm attaching.

Comment 1

[David Mandelin [:dmandelin]]

Attachment: Mostly-done patch

This patch is mostly done, so it should be OK for testing, but there are several things to note and some issues left to resolve:

- Per IRC, the spec is now to check any method that (a) has an explicitly annotated or guessed (in last position) outparam, and (b) returns nsresult, PRBool, or void. void is assumed to always succeed, so the method should write all outparams.

- A method that has (a) but not (b) is a warning -- something with outparams that we don't know how to analyze (b/c we don't know what success means).

- As a special case, constructors are never analyzed. I did this just because some of the nsCOMPtr things have a pointer arg for the last arg which is not supposed to be written to, so it was causing false positives. Let me know if this is right or not.

- There are still what I think are false positives from things like do_GetService that delegate to something and have an 'nsresult *' for their last parameter. Not sure what you want to do about these. They could be annotated with NS_INPARAM, or maybe the guessing process should not consider an nsresult * to be an outparam?

- It was necessary to make a decision on how to guess whether 'char (*a)[]' (by which I am trying to say pointer-to-array-of-char) is an outparam. I decided to treat pointers to arrays the same as pointers to pointers in the original spec.

- There are some methods for which the analysis issues a warning saying that it can't determine whether the return value is success or failure (the patch prints ASDF in the warning to make it easier to find them). I'm pretty sure this represents a bug in the script, so I want to look into it more deeply. So far, in the examples I checked, the code is a little "funny" from the outparams point of view, so besides fixing the solver bugs, there may be some design decisions needed.

Example 1: modules/libjar/nsZipArchive.cpp:1405

  static PRBool IsSymlink(unsigned char *ll)
  {
    return ((xtoint(ll+2) & S_IFMT) == S_IFLNK);
  }

It looks like the parameter isn't really an outparam. Maybe it should be a const pointer.

Example 2: content/base/src/nsScriptLoader.cpp:663

static PRBool
DetectByteOrderMark(const unsigned char* aBytes, PRInt32 aLen, nsCString& oCharset)
{
  if (aLen < 2)
    return PR_FALSE;

  switch(aBytes[0]) {
  case 0xEF:
    if (aLen >= 3 && 0xBB == aBytes[1] && 0xBF == aBytes[2]) {
      // EF BB BF                                                                   
      // Win2K UTF-8 BOM                                                            
      oCharset.Assign("UTF-8");
    }
    break;
  case 0xFE:
    if (0xFF == aBytes[1]) {
      // FE FF                                                                      
      // UTF-16, big-endian                                                         
      oCharset.Assign("UTF-16BE");
    }
    break;
  case 0xFF:
    if (0xFE == aBytes[1]) {
      // FF FE                                                                      
      // UTF-16, little-endian                                                      
      oCharset.Assign("UTF-16LE");
    }
    break;
  }
  return !oCharset.IsEmpty();
}

I don't even know if this is right. I think it's correct if we're allowed to assume the outparam is empty string on input. If so, I think it is possible to make the analysis pass this function by giving abstract semantics to IsEmpty.

Anyway, there's not much of a pattern yet, so I don't have a recommendation for now. There are only about 18 of these, so it might not be a huge deal.

Comment 2

[Benjamin Smedberg]

Ah, I think I can make your job easier: we should only guess that something is an outaparam if the method returns nsresult. No guessing when methods return PRBool or void: that requires explicit annotation.

Comment 3

[David Mandelin [:dmandelin]]

OK, I have this even more almost done, but there's one last problem that looks pretty sticky: the annotation you added doesn't actually show up in the analysis. It appears that parameter annotations in function definitions override those in function declarations. I.e., given:

  PRBool Get(..., nsIFoo **aFoo NS_OUTPARAM);
  
  PRBool Get(..., nsIFoo **aFoo) {
    // code
  }

At the call site for Get, there will be no attribute.

I'm going to ask the GCC mailing list what's going on, but in the meantime, I have the suspicion that it just doesn't work this way. I also noticed that GCC-defined attributes that add semantics to parameters are done as function parameters, which would look like this:

  PRBool Get(..., nsIFoo **aFoo) NS_OUTPARAMS("2"); // arg 2 is outparam

(Actually, GCC does __attribute__(((blah(1,2))), but that doesn't work well with macro controls, because of the variable number of arguments.) I think we could also do this, which is easier to read:

  PRBool Get(..., nsIFoo **aFoo) NS_OUTPARAMS("aFoo");

Comment 4

[David Mandelin [:dmandelin]]

Attachment: Proposed patch

OK, this is it. Note that it won't actually fix the problem for the source file in this bug report unless you have the patch for bug 433084 applied to your analysis GCC.