1374024 - Create a static analysis to detect the NS_ConvertUTF8toUTF16 and friends UAF footgun

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[(no longer active)]

Bug 1366595 is the latest reincarnation of this, I have seen it happen elsewhere in the past also.  The pattern is this.  Code does something like:

char16_t* raw = ComplexOperationProducingTemporary().get();
DoSomething(raw);

At this point raw points to freed memory.

We could add a deleted get() method to NS_ConvertUTF8toUTF16() with a && ref qualifier for example but that wouldn't work because of the legit printf case:

  printf("log: %p\n", ComplexOp().get());

I think in general calling .get() on the result of a temporary of type NS_ConvertUTF8toUTF16 is fine if it is being passed inside an argument to another function, since the usage of it is bound to the scope of that function (typically at least, unless if the callee saves it to a global) but otherwise it's a footgun by default, so we should prevent it from being called in other contexts.

Comment 1

[Andi [:andi]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #0)
> Bug 1366595 is the latest reincarnation of this, I have seen it happen
> elsewhere in the past also.  The pattern is this.  Code does something like:
> 
> char16_t* raw = ComplexOperationProducingTemporary().get();
> DoSomething(raw);
> 
> At this point raw points to freed memory.
> 
> We could add a deleted get() method to NS_ConvertUTF8toUTF16() with a && ref
> qualifier for example but that wouldn't work because of the legit printf
> case:
> 
>   printf("log: %p\n", ComplexOp().get());
> 
> I think in general calling .get() on the result of a temporary of type
> NS_ConvertUTF8toUTF16 is fine if it is being passed inside an argument to
> another function, since the usage of it is bound to the scope of that
> function (typically at least, unless if the callee saves it to a global) but
> otherwise it's a footgun by default, so we should prevent it from being
> called in other contexts.

Thanks for reporting this issue. This would prove very useful in order to prevent future issues.

Comment 2

[Tristan Bourvon]

I have a first working version of this, but before I submit it I would like to know if anyone knows about more classes I should include in the checker (beyond NS_ConvertUTF8toUTF16).

Or maybe there is a common pattern in those classes I'm missing and that could be used to detect them? (other than having a get() method, which looks too general to be used as the sole criteria).

Comment 3

[(no longer active)]

(In reply to Tristan Bourvon from comment #2)
> I have a first working version of this, but before I submit it I would like
> to know if anyone knows about more classes I should include in the checker
> (beyond NS_ConvertUTF8toUTF16).

I would say the following additional classes:

  * NS_LossyConvertUTF16toASCII
  * NS_ConvertASCIItoUTF16
  * NS_ConvertUTF16toUTF8

Nathan, do you happen to know of other similar example classes that are affected by the same pattern of bugs by any chance?

> Or maybe there is a common pattern in those classes I'm missing and that
> could be used to detect them? (other than having a get() method, which looks
> too general to be used as the sole criteria).

See the classes declared in https://searchfox.org/mozilla-central/source/xpcom/string/nsString.h.  The common pattern I have in mind is something along the lines of "classes inheriting from nsAuto(C)String which have a .get() method which can return a pointer to something that the destructor frees."

Obviously other classes with similar getter methods which return things the dtor frees are affected by the same issue, but I can't think of a great way to detect them and do anything about them.  One thing that these 4 classes have about them is the fact that people use them in patterns such as the one I described in comment 0.  While I'm sure there are other examples of other classes in our code that have similar issues, I can't think of them off the top of my head.

Comment 4

[Andi [:andi]]

Ehsan what it for now we just only build a static list with the classes that could potentially cause this problem and as a followup bug we investigate this further and possible find a pattern that can be matched through AST matchers?
Does this work for you?

Comment 5

[(no longer active)]

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #4)
> Ehsan what it for now we just only build a static list with the classes that
> could potentially cause this problem and as a followup bug we investigate
> this further and possible find a pattern that can be matched through AST
> matchers?
> Does this work for you?

Yes that sounds great!

But I don't think we should hardcode the names of the classes!  We should use attributes.  For example, let's have an attribute called MOZ_NO_GETTERS_ON_TEMPORARIES (feel free to suggest better names, just typed the first name coming to my mind here!) along the lines of the giant list we have here: https://searchfox.org/mozilla-central/rev/e8f4f51cd543f203e9cb861cecb7545ac43c836c/mfbt/Attributes.h#422

Then we can add this attribute to the four classes suggested above, and in the analysis we can apply the analysis to any class that has this attribute set on it.  That way if someone can think of another class that could use this attribute, they can simply drop this attribute on it and they will immediately start to benefit from the extra checks.  And we can use the attribute in the tests for the analysis as well.

This is generally how the other analyses we currently have get applied to the various classes in our code, and there should be many existing examples to look at.

Comment 6

[Andi [:andi]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #5)
> (In reply to Andi-Bogdan Postelnicu [:andi] from comment #4)
> > Ehsan what it for now we just only build a static list with the classes that
> > could potentially cause this problem and as a followup bug we investigate
> > this further and possible find a pattern that can be matched through AST
> > matchers?
> > Does this work for you?
> 
> Yes that sounds great!
> 
> But I don't think we should hardcode the names of the classes!  We should
> use attributes.  For example, let's have an attribute called
> MOZ_NO_GETTERS_ON_TEMPORARIES (feel free to suggest better names, just typed
> the first name coming to my mind here!) along the lines of the giant list we
> have here:
> https://searchfox.org/mozilla-central/rev/
> e8f4f51cd543f203e9cb861cecb7545ac43c836c/mfbt/Attributes.h#422
> 
> Then we can add this attribute to the four classes suggested above, and in
> the analysis we can apply the analysis to any class that has this attribute
> set on it.  That way if someone can think of another class that could use
> this attribute, they can simply drop this attribute on it and they will
> immediately start to benefit from the extra checks.  And we can use the
> attribute in the tests for the analysis as well.
> 
> This is generally how the other analyses we currently have get applied to
> the various classes in our code, and there should be many existing examples
> to look at.
Yes this is even better than having a list with classes, will do this implementation.

Comment 7

[Tristan Bourvon]

Attachment: Main checker patch

Comment 8

[Tristan Bourvon]

Attachment: Adds annotations to the classes which need the checker

Attaching that while waiting for Nathan's answer :)

Comment 9

[Tristan Bourvon]

Forgot to mention, there is a bit of noise in the main patch because I ran clang-format on Utils.h and CustomMatchers.h.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8883602 [details] [diff] [review]
Main checker patch

Review of attachment 8883602 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/clang-plugin/Checks.inc
@@ +27,4 @@
>  CHECK(RefCountedInsideLambdaChecker, "refcounted-inside-lambda")
>  CHECK(ScopeChecker, "scope")
>  CHECK(SprintfLiteralChecker, "sprintf-literal")
> +CHECK(TemporaryRefCountedGetChecker, "temporary-refcounted-get")

This has nothing to do with refcounting. Perhaps `RawTemporaryGetChecker`?

::: build/clang-plugin/TemporaryRefCountedGetChecker.cpp
@@ +47,5 @@
> +
> +  // We emit the error diagnostic indicating that we are calling get() on a
> +  // temporary.
> +  diag(MemberCall->getExprLoc(), Error, DiagnosticIDs::Error)
> +      << MemberCall->getSourceRange();

You don't need to << the source range into the diagnostic, as there is no placeholder for it to be written into.

::: build/clang-plugin/VariableUsageHelpers.cpp
@@ +19,5 @@
> +
> +  // We iterate through all the CFGBlocks, which basically means that we go over
> +  // all the possible branches of the code and therefore cover all statements.
> +  for (auto It = StatementCFG->begin(), EndIt = StatementCFG->end();
> +       It != EndIt; ++It) {

You should be able to use a standard for loop here, because the begin() and end() methods are defined on StatementCFG:

for (auto& Block : StatementCFG) {...}

@@ +24,5 @@
> +    auto Block = *It;
> +
> +    // We iterate through all the statements of the block.
> +    for (auto BlockIt = Block->begin(), BlockEnd = Block->end();
> +         BlockIt != BlockEnd; ++BlockIt) {

for (auto& BlockIt : Block) {...}

@@ +83,5 @@
> +
> +  // We find the argument number corresponding to the Arg expression.
> +  unsigned ArgNum = 0;
> +  for (auto CallArg : Call->arguments()) {
> +    if (IgnoreTrivials(Arg) == CallArg) {

We need to IgnoreTrivials on both sides of this comparison for it to work correctly, I think.

::: build/clang-plugin/VariableUsageHelpers.h
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Please add include guards to this header
#ifndef VariableUsageHelpers_h__
...etc

@@ +10,5 @@
> +namespace clang {
> +  class Stmt;
> +  class ValueDecl;
> +  class CallExpr;
> +}

We're very loose with imports in the clang plugin right now. I'd just #include "plugin.h" and be over with it.

I'd complain about the `using namespace` declarations in a header, but every compilation unit already does this due to plugin.h.

@@ +14,5 @@
> +}
> +
> +/// Returns a list of the statements where the given declaration is used as an
> +/// rvalue (within the provided function).
> +/// WARNING: incomplete behaviour/implementation for general-purpose use outside

nit: Please add a blank comment line before the WARNING

@@ +25,5 @@
> +/// Returns true if an argument from a call expression escapes the function
> +/// scope through globals/statics/other things, otherwise returns false.
> +/// If the given expression is not an argument of the call expression, returns
> +/// false.
> +/// WARNING: incomplete behaviour/implementation for general-purpose use outside

nit: Please add a blank comment line before the WARNING

::: build/clang-plugin/tests/TestTemporaryRefCountedGet.cpp
@@ +20,5 @@
> +int main() {
> +  int *test = TemporaryFunction().get(); // expected-error {{calling get() on the result of a temporary, causing potential use after free}}
> +
> +  NoEscapeFunction(TemporaryFunction().get());
> +  EscapeFunction1(TemporaryFunction().get()); // expected-error {{calling get() on the result of a temporary, causing potential use after free}}

I think detecting whether the called function allows the parameter to escape is beyond the scope of this analysis. I would prefer it if we didn't do that.

I imagine that your simple escape analysis may be useful in the future, but I think that using it in this situation might be a bit overkill. It will make the analysis's errors more unpredictable, which is very unfortunate. Asking ehsan for feedback on whether this makes sense.

@@ +24,5 @@
> +  EscapeFunction1(TemporaryFunction().get()); // expected-error {{calling get() on the result of a temporary, causing potential use after free}}
> +  int *escape;
> +  EscapeFunction2(TemporaryFunction().get(), escape); // expected-error {{calling get() on the result of a temporary, causing potential use after free}}
> +  int *escape2 = EscapeFunction3(TemporaryFunction().get()); // expected-error {{calling get() on the result of a temporary, causing potential use after free}}
> +}
\ No newline at end of file

Please also test `NS_ConvertUTF8toUTF16().get()` (as-in calling a constructor which creates the value rather than an intermediate function).

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8883607 [details] [diff] [review]
Adds annotations to the classes which need the checker

Review of attachment 8883607 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/string/nsString.h
@@ +50,4 @@
>  /**
>   * A helper class that converts a UTF-16 string to ASCII in a lossy manner
>   */
> +class MOZ_NO_GET_ON_TEMPORARIES NS_LossyConvertUTF16toASCII

We might want to make this property be inherited from base classes, and place this annotation on `nsTString_CharT`, as it's usually wrong to call `get` on any temporary string. http://searchfox.org/mozilla-central/rev/e8f4f51cd543f203e9cb861cecb7545ac43c836c/xpcom/string/nsTString.h#20

Comment 12

[Nathan Froyd [:froydnj]]

Looks like we have a better solution than hardcoding class names!  I think Michael's suggestion of adding this analysis for all string classes would be a good start.

I would suggest two things:

* The attribute shouldn't be restricted to analyzing "get" calls, but should take an additional argument for the name of the restricted method (this might mean a single class has multiple such attributes for multiple methods?).  Overloading makes this a little tricky, but I submit the best course of action is just the method name (sans arguments of any kind) and if that proves insufficient at some later date, we can revisit.
* The documentation for any such attribute prohibiting temporaries needs to make very clear in the documentation why we can't use ref-qualifiers on the method.  The attribute name might also need to be a little clearer about what kind of temporaries are prohibited ("dangling" seems like as good a description as any).

Comment 13

[Tristan Bourvon]

(In reply to Michael Layzell [:mystor] from comment #10)
> Comment on attachment 8883602 [details] [diff] [review]
> Main checker patch
> 
> Review of attachment 8883602 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/clang-plugin/Checks.inc
> @@ +27,4 @@
> >  CHECK(RefCountedInsideLambdaChecker, "refcounted-inside-lambda")
> >  CHECK(ScopeChecker, "scope")
> >  CHECK(SprintfLiteralChecker, "sprintf-literal")
> > +CHECK(TemporaryRefCountedGetChecker, "temporary-refcounted-get")
> 
> This has nothing to do with refcounting. Perhaps `RawTemporaryGetChecker`?

My brain got confused, thanks for pointing that out.

> ::: build/clang-plugin/TemporaryRefCountedGetChecker.cpp
> @@ +47,5 @@
> > +
> > +  // We emit the error diagnostic indicating that we are calling get() on a
> > +  // temporary.
> > +  diag(MemberCall->getExprLoc(), Error, DiagnosticIDs::Error)
> > +      << MemberCall->getSourceRange();
> 
> You don't need to << the source range into the diagnostic, as there is no
> placeholder for it to be written into.


> ::: build/clang-plugin/tests/TestTemporaryRefCountedGet.cpp
> @@ +20,5 @@
> > +int main() {
> > +  int *test = TemporaryFunction().get(); // expected-error {{calling get() on the result of a temporary, causing potential use after free}}
> > +
> > +  NoEscapeFunction(TemporaryFunction().get());
> > +  EscapeFunction1(TemporaryFunction().get()); // expected-error {{calling get() on the result of a temporary, causing potential use after free}}
> 
> I think detecting whether the called function allows the parameter to escape
> is beyond the scope of this analysis. I would prefer it if we didn't do that.
> 
> I imagine that your simple escape analysis may be useful in the future, but
> I think that using it in this situation might be a bit overkill. It will
> make the analysis's errors more unpredictable, which is very unfortunate.
> Asking ehsan for feedback on whether this makes sense.

I'll leave the decision to both of you, but the way this escape check is implemented leaves very little room for false positives (the only case I can think of is if the parameter is reassigned in the function before escaping it... which should be fairly rare). It will mostly be making errors by missing stuff because it is too simplistic. So in the end, this just adds a bit more coverage for the checker, and can allow us to catch a bit more errors, which as we've seen can be pretty bad.

> @@ +24,5 @@
> > +  EscapeFunction1(TemporaryFunction().get()); // expected-error {{calling get() on the result of a temporary, causing potential use after free}}
> > +  int *escape;
> > +  EscapeFunction2(TemporaryFunction().get(), escape); // expected-error {{calling get() on the result of a temporary, causing potential use after free}}
> > +  int *escape2 = EscapeFunction3(TemporaryFunction().get()); // expected-error {{calling get() on the result of a temporary, causing potential use after free}}
> > +}
> \ No newline at end of file
> 
> Please also test `NS_ConvertUTF8toUTF16().get()` (as-in calling a
> constructor which creates the value rather than an intermediate function).

I realized that when looking for occurences of calls to .get() yesterday evening. Very good catch, as this is by far the most common use of the converters and it wasn't handled by the previous patch. Now, any temporary value will be matched against.

Comment 14

[Tristan Bourvon]

Sorry, missed a comment:

> ::: build/clang-plugin/TemporaryRefCountedGetChecker.cpp
> @@ +47,5 @@
> > +
> > +  // We emit the error diagnostic indicating that we are calling get() on a
> > +  // temporary.
> > +  diag(MemberCall->getExprLoc(), Error, DiagnosticIDs::Error)
> > +      << MemberCall->getSourceRange();
> 
> You don't need to << the source range into the diagnostic, as there is no
> placeholder for it to be written into.

This is useful because it underlines the whole expression with ~~~, making it easier to see where the error lies.

Comment 15

[Tristan Bourvon]

Attachment: 1374024.patch

Comment 16

[Tristan Bourvon]

Attachment: 1374024 - annotations.patch

Comment 17

[(no longer active)]

Comment on attachment 8883936 [details] [diff] [review]
1374024.patch

Review of attachment 8883936 [details] [diff] [review]:
-----------------------------------------------------------------

I skimmed the patch and it looks great overall.  I'll leave the detailed review to Michael.  On the escape analysis question, I can't think of a false positive possibility.  I do think we should emit a note diagnostic when we modify our behavior as a result of the escape analysis as noted below in order to make the behavior of the analysis more easily understandable to developers.

Thanks for your great work on this so far!

::: build/clang-plugin/VariableUsageHelpers.cpp
@@ +102,5 @@
> +  // get() methods only return pointers, but more cases should probably be
> +  // handled when we want to use this function more broadly.
> +  if (!Arg->getType()->isPointerType()
> +      || !Param->getType()->isPointerType()) {
> +    return false;

It is rather scary to return false for the cases where the implementation here is incomplete.  Aborting or something along those line may be a more appropriate course of action to ensure nobody will call this in an inappropriate context in the future.

@@ +134,5 @@
> +        if (!ParamDeclaration->getType()->isReferenceType()) {
> +          continue;
> +        }
> +
> +        return true;

It would be really nice to record the SourceLocation of the Decl's that we return true for here and inside DanglingOnTemporaryChecker, emiting some note diagnostics for them like "value may escape from the callee here".  I think this will help make it more obvious to people when the analysis rejects code due to the escape analysis.

::: mfbt/Attributes.h
@@ +526,4 @@
>   * MOZ_NEEDS_MEMMOVABLE_MEMBERS: Applies to class declarations where each member
>   *   must be safe to move in memory using memmove().  MOZ_NON_MEMMOVABLE types
>   *   used in members of these classes are compile time errors.
> + * MOZ_NO_GET_ON_TEMPORARIES: Applies to class declarations where a .get()

Did you see Nathan's comment (comment 12)?  I think it may be valuable to not restrict the analysis to get() functions.  We may want a second attribute which would go on the methods to which the analysis would apply, so it would look for MOZ_NO_METHOD_ON_TEMPORARIES attributes to detect which objects to analyze, and would look for MOZ_RESTRICTED_METHOD_ON_TEMPORARIES attribute (or some similar name) to analyze which methods to apply it to.

Comment 18

[Tristan Bourvon]

(In reply to :Ehsan Akhgari (Away 7/10-7/17; needinfo please, extremely long backlog) from comment #17)
> Comment on attachment 8883936 [details] [diff] [review]
> 1374024.patch
> 
> Review of attachment 8883936 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I skimmed the patch and it looks great overall.  I'll leave the detailed
> review to Michael.  On the escape analysis question, I can't think of a
> false positive possibility.  I do think we should emit a note diagnostic
> when we modify our behavior as a result of the escape analysis as noted
> below in order to make the behavior of the analysis more easily
> understandable to developers.
> 
> Thanks for your great work on this so far!

Well there is the reassignment one I talked about in my reply to Michael's review, but yeah I'm glad nobody thinks of another one.

> ::: build/clang-plugin/VariableUsageHelpers.cpp
> @@ +102,5 @@
> > +  // get() methods only return pointers, but more cases should probably be
> > +  // handled when we want to use this function more broadly.
> > +  if (!Arg->getType()->isPointerType()
> > +      || !Param->getType()->isPointerType()) {
> > +    return false;
> 
> It is rather scary to return false for the cases where the implementation
> here is incomplete.  Aborting or something along those line may be a more
> appropriate course of action to ensure nobody will call this in an
> inappropriate context in the future.

Oh yeah you're right, this is scary.

> ::: mfbt/Attributes.h
> @@ +526,4 @@
> >   * MOZ_NEEDS_MEMMOVABLE_MEMBERS: Applies to class declarations where each member
> >   *   must be safe to move in memory using memmove().  MOZ_NON_MEMMOVABLE types
> >   *   used in members of these classes are compile time errors.
> > + * MOZ_NO_GET_ON_TEMPORARIES: Applies to class declarations where a .get()
> 
> Did you see Nathan's comment (comment 12)?  I think it may be valuable to
> not restrict the analysis to get() functions.  We may want a second
> attribute which would go on the methods to which the analysis would apply,
> so it would look for MOZ_NO_METHOD_ON_TEMPORARIES attributes to detect which
> objects to analyze, and would look for MOZ_RESTRICTED_METHOD_ON_TEMPORARIES
> attribute (or some similar name) to analyze which methods to apply it to.

Actually I changed this, but forgot to modify the associated comment, which happens to be what you've read apparently. :)

Comment 19

[Tristan Bourvon]

Attachment: 1374024.patch

Ehsan, does that look like what you wanted?

I've also added a very small sub-checker to prevent using the annotation on && ref-qualified methods. Thanks Andi for the suggestion!

Comment 20

[(no longer active)]

Comment on attachment 8884284 [details] [diff] [review]
1374024.patch

Review of attachment 8884284 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, looking at the test changes, it looks much better now!  Thanks a lot.

Comment 21

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8884284 [details] [diff] [review]
1374024.patch

Review of attachment 8884284 [details] [diff] [review]:
-----------------------------------------------------------------

This is fantastic! Thanks :).

r=me with a few super small nit comments.

::: build/clang-plugin/CustomMatchers.h
@@ +105,4 @@
>  /// This matcher will match any function declaration that is marked to prohibit
>  /// calling AddRef or Release on its return value.
>  AST_MATCHER(FunctionDecl, hasNoAddRefReleaseOnReturnAttr) {
> +  return hasCustomAnnotation(&Node, "moz_no_addref_release_on_return");

super nit: Is there any chance you could run clang-format on these files and commit those changes in a different commit before adding these changes? It would be nice to have separate blame on them.

::: build/clang-plugin/DanglingOnTemporaryChecker.cpp
@@ +15,5 @@
> +      cxxMethodDecl(
> +          // which is marked as no dangling on temporaries,
> +          noDanglingOnTemporaries(),
> +          isRValueRefQualified(),
> +          decl().bind("methodDecl")),

nit: Please change this binding name to something which makes it more clear that it's a method declaration we want to complain about. Perhaps `invalidMethodDecl`?

::: build/clang-plugin/VariableUsageHelpers.cpp
@@ +99,5 @@
> +
> +  // We want both the argument and the parameter to be of pointer type.
> +  // FIXME: this is enough for the DanglingOnTemporaryChecker, because the
> +  // analysed methods only return pointers, but more cases should probably be
> +  // handled when we want to use this function more broadly.

This is really unfortunate. I would want to emit a diagnostic error when this condition isn't met, as people can add this annotation to a method and would expect it to work rather than simply never complaining silently. I'm not sure what the best way to do that from this place in the code is, however.

::: build/clang-plugin/tests/TestDanglingOnTemporary.cpp
@@ +32,5 @@
> +
> +  int *escape;
> +  EscapeFunction2(TemporaryFunction().get(), escape); // expected-error {{calling `get` on a temporary, potentially allowing use after free of the raw pointer}}
> +  int *escape2 = EscapeFunction3(TemporaryFunction().get()); // expected-error {{calling `get` on a temporary, potentially allowing use after free of the raw pointer}}
> +}

Can you add a test for the function that NS_ConvertUTF8toUTF16().get() is being passed to not being defined in this compilation unit for completeness?

::: mfbt/Attributes.h
@@ +530,5 @@
> + *   a pointer that is freed when the destructor of the class is called. This
> + *   prevents these methods from being called on temporaries of the class,
> + *   reducing risks of use-after-free.
> + *   This attribute cannot be applied to && methods.
> + *   The solution of adding a deleted overload of the method with a && ref

Can you reword this paragraph a bit? Perhaps something like:

In some cases, adding a deleted &&-qualified overload is too restrictive as this method should still be callable as a non-escaping argument to another function. This annotation can be used in those cases.

Comment 22

[Tristan Bourvon]

Attachment: 1374024.patch

So after checking we still have 0 matches in our code, which is good news, because it means that we don't have this kind of issue, and also that we can land the patch directly :)

Comment 23

[Tristan Bourvon]

Attachment: 1374024 - format.patch

Comment 24

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8885171 [details] [diff] [review]
1374024.patch

Review of attachment 8885171 [details] [diff] [review]:
-----------------------------------------------------------------

r=me again, but it looks like the formatting is still in this patch.

Comment 25

[Tristan Bourvon]

Attachment: 1374024.patch

Woops sorry, I thought you just wanted me to re-run clang-format and put the diff in a new patch, not to move the formatting chunks from the first patch to a format patch. Here you go!

Comment 26

[Tristan Bourvon]

Attachment: 1374024 - format.patch

Comment 27

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8885690 [details] [diff] [review]
1374024.patch

Review of attachment 8885690 [details] [diff] [review]:
-----------------------------------------------------------------

Carrying over r+ under the assumption that you didn't make any other changes.

Comment 28

[Andi [:andi]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a49d403a9a49313fbeabbb9d84203fe33701173
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f768b83c9288c751c1e8a90870c51154b8b798b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1eb2623a06c8e16a2f9609b673653fcddab882cc

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1eb2623a06c8e16a2f9609b673653fcddab882cc

Comment 29

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=113933345&repo=mozilla-inbound

Comment 30

[Andi [:andi]]

(In reply to Carsten Book [:Tomcat] from comment #29)
> sorry had to back this out for bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=113933345&repo=mozilla-
> inbound

Thanks for doing this, we know about the issue!

Comment 31

[Tristan Bourvon]

Thanks, this is being taken care of.

Comment 32

[Carsten Book [:Tomcat]]

backed out again for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=113997046&repo=mozilla-inbound

Comment 33

[Andi [:andi]]

remote: View your changes here:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2710ae26000ce21716601946205a2a4691caeffa
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/003d87d355859d5443daffcb1dfcf41b89b21a46
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/32452cb511f50efb743f80d4f9af6f03570b00be
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=32452cb511f50efb743f80d4f9af6f03570b00be

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2710ae26000c
https://hg.mozilla.org/mozilla-central/rev/003d87d35585
https://hg.mozilla.org/mozilla-central/rev/32452cb511f5

Comment 35

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Tristan, it looks like you are the assignee on this bug, but under a different email. Are you still working on this bug? If not, would you please unassign yourself?

Comment 36

[Firefox Bug Husbandry Bot]

Sorry for the spurious NI request, missed that this bug had been resolved.