Skip to content

Simplify marshalling of StartupInfo struct used in Jumplist creation. - #7580

Merged
Aditya Patwardhan (adityapatwardhan) merged 2 commits into
PowerShell:masterfrom
powercode:CrashInJumplist
Aug 24, 2018
Merged

Simplify marshalling of StartupInfo struct used in Jumplist creation.#7580
Aditya Patwardhan (adityapatwardhan) merged 2 commits into
PowerShell:masterfrom
powercode:CrashInJumplist

Conversation

@powercode

@powercode Staffan Gustafsson (powercode) commented Aug 21, 2018

Copy link
Copy Markdown
Collaborator

PR Summary

#7572

The current code mixes Ansi version of GetStartupInfoA with Unicode marshalling. This leads to a heap corruption that manifests in a crash a few lines down.

This commit simplifies the interop struct, using IntPtr instead of string since we have no usage for the strings. With that change, all fields are blitable, making the marshaller both simpler and faster.

GetStartupInfoA is documented to throw and exception, not set last error, So I changed to SetLastError = false.

GetStartupInfoW does not fail, indicated by the void return type.
I also made the reserved fields private, and all fields readonly.

PR Checklist

@iSazonov Ilya (iSazonov) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one minor comment.

internal static extern void GetStartupInfo(out StartUpInfo lpStartupInfo);

[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)]
[StructLayout(LayoutKind.Sequential)]

@iSazonov Ilya (iSazonov) Aug 22, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment why we use IntPtr for strings - this help us avoid problems in future.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants