Closed Bug 1662720 Opened 6 years ago Closed 5 years ago

Do not distinguish _platform_memmove$VARIANT$* in signatures

Categories

(Socorro :: Signature, enhancement, P2)

Product:
Socorro
Component:
Signature
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: sg, Assigned: willkg)

References

Details

Attachments

(1 file)

pr 5571: bug 1662720: add *$VARIANT$* symbols to irrelevant list
53 bytes, text/x-github-pull-request
Details | Review

Since it's highly unlikely there is a bug in some of the _platform_memmove$VARIANT$* itself, and all crashes containing this signature part are likely to happen for all of the variants, it would be preferable not to distinguish those.

Either they should be mapped to a common name such as _platform_memmove, or they could be completely skipped in signature generation. I am not sure but there might also be a variant prefixed with two underscores instead of one. Generally, I think the former option would be preferable, but I am not sure if this is implemented.

FWIW, Bug 1309102 at some point already requested adding these to the skiplist, but ended up adding them to the prefix list, which is only moderately helpful.

In bug #1309102, it was discovered one of them was already in the prefix list, so it generalized that. I understand the issue you're having now.

There isn't a way to map a group of functions to a common name, but maybe that's something signature generation should be able to do. Are there other functions that have similar platform variants such that this would be helpful for other things?

Flags: needinfo?(sgiesecke)
No longer depends on: 1309102
See Also: → 1309102

There are some functions similarly containing $VARIANT, yes: https://crash-stats.mozilla.org/search/?signature=~%24VARIANT&date=%3E%3D2020-06-02T13%3A18%3A00.000Z&date=%3C2020-09-02T13%3A18%3A00.000Z&_facets=signature&_sort=-date&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature
including

  • _platform_memset_pattern16
  • _platform_bzero
  • _OSAtomicCompareAndSwap32

And indeed there are also variants with two prefixing underscores in that list.

Flags: needinfo?(sgiesecke)
Blocks: 1659938

What's the urgency to look into this? Is it blocking other work?

Type: task → enhancement

On the linked sec-bug, I had some trouble to put together a list of all relevant signatures with confidence, because there were so many of them. It's possible that other issues are overseen because the individual numbers of crashes are very low due to them being spread over several signatures. But it's not blocking anything I am aware of.

Huh. I don't see a linked bug. Are you sure you linked it to this bug?

I'll try to look into it next week.

Assignee: nobody → willkg
Status: NEW → ASSIGNED
Priority: -- → P2

I spent some time on this today. I'm wondering a few things:

  1. First option. We could write a signature generation frame normalization rule that nixes everything after $VARIANT$ for symbols that have them. Then _platform_memset$VARIANT$Haswell would become _platform_memset$VARIANT$.

    Is it important to denote that it's a variant thing in the signature? If so, we could do something like _platform_memset$VARIANT$ or _platform_memset$V$. If not, then we could drop $VARIANT$ and everything after it and our example would become _platform_memset.

    Since we're keying off of $VARIANT$ existing in the symbol, this would work for all $VARIANT$ symbols--we wouldn't have fix them as we discover them.

    We wouldn't add additional prefix/irrelevant rules, so whatever is happening now would continue to happen in regards to whether the symbol ends up in the signature.

  2. Second option. We could add these symbols to the irrelevant list as we discover them. For now, we'd move _platform_memset$VARIANT$ and __platform_memset$VARIANT$ and some of the other ones we've noticed in this bug to the irrelevant list.

    We'd have to make similar adjustments as new issues pop up.

  3. Third option. We add a rule to the irrelevant list that drops any symbols with $VARIANT$ in them. Since this keys off of $VARIANT$, we make this change once and don't have to keep fixing them as we discover them.

    I looked at some of the crash reports where the signature consists solely of one of these symbols. For example, since _platform_memset_pattern16$VARIANT$ isn't in the prefix list, signature generation ends on that symbol, so it's the only symbol in the signature. For the crash reports I looked at, there are other frames after that, so seems like this change would improve those signatures.

    https://crash-stats.mozilla.org/signature/?signature=_platform_memset_pattern16%24VARIANT%24Merom&date=%3E%3D2020-06-02T13%3A18%3A00.000Z&date=%3C2020-09-02T13%3A18%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=startup_crash&_sort=-date&page=1#reports

Gabriele, Simon: What do you think? Based on what Simon is saying, I'm thinking option 3 seems like the right way to go here.

Flags: needinfo?(sgiesecke)
Flags: needinfo?(gsvelto)

Yes, Option 3 sounds good to me.

Generally, I do not completely understand when it's better to put a symbol on the irrelevant vs. the prefix list. Somehow I had a preference for the prefix list, since in particular in large functions (of which we have many) there might be unrelated crashes in different functions called within the same function in theory, which are only distinguished if the called functions are on the prefix rather than the irrelevant list. But maybe that's not of practical relevance.

Flags: needinfo?(sgiesecke)

I think you're right on: we should prefer the prefix list to the irrelevant list. The irrelevant list is helpful for a couple of situations:

  1. cases where the left-most parts of the signature aren't interesting because they're things like error handling
  2. cases where the symbols are platform-specific and cause crash reports across different platforms to end up with different signatures

I did a pass on the signature generation documentation to clarify this.

I'm OK with option 3. Since all these functions are libc stuff they're usually of low-ish value, just skipping them and picking the following frame would probably lead to better signatures - and save time manually hunting down all the variants when filing crashes.

Flags: needinfo?(gsvelto)

Awesome--I'll work on this tomorrow. Thank you both!

(In reply to Will Kahn-Greene [:willkg] ET needinfo? me from comment #8)

I think you're right on: we should prefer the prefix list to the irrelevant list. The irrelevant list is helpful for a couple of situations:

  1. cases where the left-most parts of the signature aren't interesting because they're things like error handling
  2. cases where the symbols are platform-specific and cause crash reports across different platforms to end up with different signatures

I did a pass on the signature generation documentation to clarify this.

I guess you are referring to https://github.com/mozilla-services/socorro/pull/5569. That looks excellent, thanks!

This was deployed to prod in bug #1665038.

Simon: Do we want to reprocess some group of these crash reports? If so, let me know the specifics and I can do that today.

Flags: needinfo?(sgiesecke)

Thanks!

For now, I think it's not necessary to reprocess the existing crash reports, and it's just good that new crash reports use the new signatures.

Flags: needinfo?(sgiesecke)

Sounds good! Marking as FIXED.

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: