Fix 5265 and 5307 #411

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
4 participants
@robmen
Member

robmen commented Feb 18, 2017

Prevent bad use of ".." in payload names in the Compiler and Binder.
Fixes wixtoolset/issues#5265

Correctly set and document SystemFolder and System64Folder in Burn
Fixes wixtoolset/issues#5307

robmen added some commits Feb 18, 2017

Correctly set and document SystemFolder and System64Folder in Burn
The values for SystemFolder and System64Folder were all mixed up. This
Fixes that then adds missing and fixes existing documentation about
these two Burn variables.

Fixes wixtoolset/issues#5307
@@ -1897,26 +1897,35 @@ static HRESULT InitializeVariableSystemFolder(
BOOL f64 = (BOOL)dwpData;
WCHAR wzSystemFolder[MAX_PATH] = { };
-#ifndef _WIN64

This comment has been minimized.

@rseanhall

rseanhall Feb 18, 2017

Member

This code used to return the right answer when running on a 32-bit OS, and swap SystemFolder with System64Folder on a 64-bit OS. Now it returns the right answer on 32-bit and 64-bit. Theoretically, someone could have been assuming that they're always deploying to a 64-bit OS (because their org's IT takes care of that) and then took a dependency on them being swapped. Are we justifying this breaking change by preferring the ability to support both 32-bit and 64-bit OS's? Should we have a warning somewhere?

@rseanhall

rseanhall Feb 18, 2017

Member

This code used to return the right answer when running on a 32-bit OS, and swap SystemFolder with System64Folder on a 64-bit OS. Now it returns the right answer on 32-bit and 64-bit. Theoretically, someone could have been assuming that they're always deploying to a 64-bit OS (because their org's IT takes care of that) and then took a dependency on them being swapped. Are we justifying this breaking change by preferring the ability to support both 32-bit and 64-bit OS's? Should we have a warning somewhere?

This comment has been minimized.

@robmen

robmen Feb 19, 2017

Member

I'm fine if we want to say this needs to move to WiX v4.0 because it is "a fix breaking today's behavior". Adding warning doesn't make sense to me.

@robmen

robmen Feb 19, 2017

Member

I'm fine if we want to say this needs to move to WiX v4.0 because it is "a fix breaking today's behavior". Adding warning doesn't make sense to me.

This comment has been minimized.

@rseanhall

rseanhall Feb 19, 2017

Member

I'm leaning towards taking it anyway.

@rseanhall

rseanhall Feb 19, 2017

Member

I'm leaning towards taking it anyway.

This comment has been minimized.

@robmen

robmen Feb 19, 2017

Member

Sounds like @rseanhall, you've come to the same conclusion I did in my blog post:

What if this fix breaks backwards compatbility. Then I remind myself this code never returns the right answer. Raymond Chen just wrote about a similar problem. Although our issue isn't a bluescreen or a crash. Hmmm... Nope, I'm still fixing it tonight.

😃

@robmen

robmen Feb 19, 2017

Member

Sounds like @rseanhall, you've come to the same conclusion I did in my blog post:

What if this fix breaks backwards compatbility. Then I remind myself this code never returns the right answer. Raymond Chen just wrote about a similar problem. Although our issue isn't a bluescreen or a crash. Hmmm... Nope, I'm still fixing it tonight.

😃

This comment has been minimized.

@barnson

barnson Feb 20, 2017

Member

It's not likely to break typical uses and System64Folder wasn't documented, so I'm OK with the change but it should probably get highlighted in release notes.

@barnson

barnson Feb 20, 2017

Member

It's not likely to break typical uses and System64Folder wasn't documented, so I'm OK with the change but it should probably get highlighted in release notes.

@rseanhall

This comment has been minimized.

Show comment
Hide comment
@rseanhall

rseanhall Feb 25, 2017

Member

Please make a pull request for wix4

Member

rseanhall commented Feb 25, 2017

Please make a pull request for wix4

@robmen robmen referenced this pull request in wixtoolset/wix4 Feb 26, 2017

Merged

Fix 5265 and 5307 #231

@robmen

This comment has been minimized.

Show comment
Hide comment
@robmen

robmen Feb 26, 2017

Member

Port done.

Member

robmen commented Feb 26, 2017

Port done.

@rseanhall rseanhall self-assigned this Feb 26, 2017

@rseanhall rseanhall closed this in cfe48e6 Feb 26, 2017

@robmen robmen deleted the robmen:5265-5307-burn-fixes branch Mar 1, 2017

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