Closed Bug 1240315 Opened 4 years ago Closed 4 years ago

Add AppInit_DLLs to crash report annotations at startup

Categories

(Toolkit :: Startup and Profile System, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: aklotz, Assigned: aklotz)

References

Details

Attachments

(1 file, 1 obsolete file)

We want to do this early enough such that the annotations are present if a crash happens during startup due to an injected dll.

I've run the privacy aspect past vladan and we agreed that we'd drop the path components off of most DLLs unless they show up in %ProgramFiles% or %TEMP%, where we substitute those environment variables for the real paths.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8708713 - Flags: review?(jmathies)
Comment on attachment 8708713 [details] [diff] [review]
Patch

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

::: widget/windows/WinUtils.cpp
@@ +1714,5 @@
> +  if (longResult == 0 || longResult > MAX_PATH - 1) {
> +    return false;
> +  }
> +  aOutput.SetLength(MAX_PATH + 1);
> +  if (!PathUnExpandEnvStringsW(longBuffer, aOutput.BeginWriting(), MAX_PATH)) {

does this return a null terminated string? msdn docs don't appear to say. Is this why you're going the 'Truncate to correct length' dance below?

@@ +1774,5 @@
> +    DWORD loadAppInitDLLsLen = sizeof(loadAppInitDLLs);
> +    status = RegQueryValueExW(hkey, kLoadAppInitDLLs, nullptr,
> +                              nullptr, (LPBYTE)&loadAppInitDLLs,
> +                              &loadAppInitDLLsLen);
> +    if (status) {

nit - 

status != ERROR_SUCCESS

plus below

::: widget/windows/WinUtils.h
@@ +416,5 @@
>  
>    static bool ShouldHideScrollbars();
>  
> +  static bool SanitizePath(const wchar_t* aInputPath, nsAString& aOutput);
> +  static bool GetAppInitDLLs(nsAString& aOutput);

nit - block comments please.
Attachment #8708713 - Flags: review?(jmathies) → review+
Attached patch Patch (r2)Splinter Review
Carrying forward r+.

(In reply to Jim Mathies [:jimm] from comment #2)
> Comment on attachment 8708713 [details] [diff] [review]
> Patch
> 
> Review of attachment 8708713 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/WinUtils.cpp
> @@ +1714,5 @@
> > +  if (longResult == 0 || longResult > MAX_PATH - 1) {
> > +    return false;
> > +  }
> > +  aOutput.SetLength(MAX_PATH + 1);
> > +  if (!PathUnExpandEnvStringsW(longBuffer, aOutput.BeginWriting(), MAX_PATH)) {
> 
> does this return a null terminated string? msdn docs don't appear to say. Is
> this why you're going the 'Truncate to correct length' dance below?

It should be null-terminated, yes, but we don't know its actual length other than it's less than MAX_PATH, hence the truncate dance.

> 
> @@ +1774,5 @@
> > +    DWORD loadAppInitDLLsLen = sizeof(loadAppInitDLLs);
> > +    status = RegQueryValueExW(hkey, kLoadAppInitDLLs, nullptr,
> > +                              nullptr, (LPBYTE)&loadAppInitDLLs,
> > +                              &loadAppInitDLLsLen);
> > +    if (status) {
> 
> nit - 
> 
> status != ERROR_SUCCESS
> 
> plus below

Fixed all occurrences.

> 
> ::: widget/windows/WinUtils.h
> @@ +416,5 @@
> >  
> >    static bool ShouldHideScrollbars();
> >  
> > +  static bool SanitizePath(const wchar_t* aInputPath, nsAString& aOutput);
> > +  static bool GetAppInitDLLs(nsAString& aOutput);
> 
> nit - block comments please.

Fixed.
Attachment #8708713 - Attachment is obsolete: true
Attachment #8726322 - Flags: review+
Depends on: 1253566
https://hg.mozilla.org/mozilla-central/rev/99ba8641d61f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.