[wix-devs] #3640 - Burn will use incorrect payload if local

[Sean Hall]

1. I didn't measure it. It's one less API call per file, so it has to be
faster :)

>  If I understand correctly, OnResolveSource is essentially replaced by
OnCacheAcquireResolving when args.fFoundLocal == FALSE

2. No, OnCacheAcquireResolving is called for every payload that is being
acquired. If the BA wants to block the download, it can change the action
to NONE from OnCacheAcquireResolving or it can specify COPY from
OnCacheAcquireBegin.

>  Most (all?) BAs prompt the user for source during OnResolveSource

3. That's probably because that was the only time during Apply that the
engine would allow the BA to modify its state :). I understand that it
would be ideal if the BA could set source from OnCacheAcquireResolving but
like I said, we need to solve the string problem for that. The main problem
here is the local source string, we could probably allow setting the
download source but there's not currently a way to lock down the engine
like that.

4. I do think that with the current state of the PR, the BA would prompt
for source on failure from OnCacheAcquireComplete and then retry.

On Thu, Apr 8, 2021 at 2:11 PM Rob Mensching <rob at firegiant.com> wrote:

> These are great problems to get solved in Burn.
>
>
> > During Detect, count a package as cached if any of a package's payloads
> exist without checking the file size. This makes Detect faster.
>
> 1. Just curious: Is it noticeably faster? I thought checking file size
> would be a simple NTFS directory table lookup. It must be a thing when
> there are many, many, many loose payloads, right?
>
>
> > then the engine will now automatically try to download it if it has a
> DownloadUrl
>
> 2. FYI, this was a (probably undocumented) requirement that the BA was
> always called before the engine would go to the internet. That requirement
> came from the early design days of Burn when internet downloads were more
> costly/less prolific so it's probably okay to toss it. I just wanted to
> raise it in case it informed/changed anything.
>
>
> > It is not allowed in the new OnCacheAcquireResolving event, but only
> because we don't have a good way for the BA to send strings to the engine
> to interact with a BA event.
>
> 3. Most (all?) BAs prompt the user for source during OnResolveSource. If I
> understand correctly, OnResolveSource is essentially replaced by
> OnCacheAcquireResolving when args.fFoundLocal == FALSE. If so, that's the
> most likely case where set source would be called (after the user was
> prompted for the new source) and thus I think we should make that possible
> and as easy as possible. Unless the correct solution is to use
> OnCacheAcquireComplete, if so, see the next point.
>
>
> > If the web server requires authentication, the BA could set that for
> every payload in OnCacheAcquireBegin
>
> 4. Similar to the above issue, the BA is likely to prompt the user for
> credentials on demand (in this case after fail to get a file due to
> authentication required). If the solution is to handle failure in
> OnCacheAcquireComplete by setting the source credentials then retrying, I
> expect that's cool.
>
>
> -----Original Message-----
> From: wix-devs <wix-devs-bounces at lists.wixtoolset.org> On Behalf Of Sean
> Hall via wix-devs
> Sent: Tuesday, April 6, 2021 12:59 PM
> To: WiX Toolset Developer Mailing List <wix-devs at lists.wixtoolset.org>
> Cc: Sean Hall <r.sean.hall at gmail.com>
> Subject: [wix-devs] #3640 - Burn will use incorrect payload if local
>
> I have implemented a solution for this at
> https://github.com/wixtoolset/burn/pull/48. There's still some work left
> to get the progress right. This pull request is fixing some design flaws:
>
> During Detect, a payload was assumed as cached if the file existed and had
> the right size. In v3 and v4, this could have been a false positive if it
> had the wrong hash. In v3, this could have been a false negative if the
> payload was supposed to be verified through Authenticode. This partial
> verification was done because full verification could make Detect take too
> long.
>
> When trying to acquire a payload and the engine found a suitable file in
> one of its probe paths, the BA never got a chance to change the source even
> if the payload verification failed.
>
> The engine made it impossible for the BA to request that an embedded be
> downloaded instead of acquired through the container (#5253).
>
>
> The fixes for these flaws are:
>
> During Detect, count a package as cached if any of a package's payloads
> exist without checking the file size. This makes Detect faster.
>
> Cache planning is done at the package level instead of the payload level,
> which ensures that all payloads for packages being cached go through full
> verification. This also allows the BA to request that an embedded payload
> is downloaded instead of extracted from a container.
>
> The BA can set the source during the OnCacheAcquireBegin/Complete events,
> which are always sent to the BA if the payload wasn't already present and
> verified.
>
>
> The API has changed between the engine and the BA, full details can be
> seen in the PR. The OnResolveSource event has been removed. This event was
> basically doing two things: allowing the BA to request that the BA be
> downloaded and allowing the BA to set the source. A new
> OnCacheAcquireResolving event has been added.
>
> For the request to download, this should be needed rarely after this pull
> request. If the payload can't be found locally and isn't embedded in a
> container, then the engine will now automatically try to download it if it
> has a DownloadUrl. That being said, the BA can request to download from
> OnCacheAcquireBegin or the new OnCacheAcquireResolving event.
>
> For setting the source, this is now allowed in OnCacheAcquireBegin and
> OnCacheAcquireComplete. It is not allowed in the new
> OnCacheAcquireResolving event, but only because we don't have a good way
> for the BA to send strings to the engine to interact with a BA event.
>
> Some examples:
>
> 1. Uncompressed bundle, web download
>
> In this scenario, the user downloads the bundle .exe by itself and
> everything is downloaded as needed. All payloads have a DownloadUrl. In v3,
> the BA would have to request Download in OnResolveSource for every payload.
> With this PR, no BA code is required because the engine will automatically
> try to use the DownloadUrl. If the web server requires authentication, the
> BA could set that for every payload in OnCacheAcquireBegin or like v3 could
> just use the BA manifest to go through all the payloads and set them before
> Apply.
>
> 2. Uncompressed bundle, DVD layout
>
> In this scenario, the user runs the bundle .exe from a DVD that has all
> the payloads next to it. There should be no change in behavior here, the
> engine should find all payloads next to the bundle with no BA code required.
>
> 3. Fully compressed bundle
>
> In this scenario, the bundle was installed. The user modifies the
> installation and installs a package not already present. In v3, the only
> way to get the required payload would be to provide the original bundle.
> With this PR, the BA could set the DownloadUrl during OnCacheAcquireBegin
> and request Download to avoid the engine trying to locate the original
> bundle. There would be a corresponding change in Core that allows the
> authoring of a DownloadUrl for embedded payloads so that the BA would only
> have to request download during OnCacheAcquireBegin or
> OnCacheAcquireResolving.
>
> An aside:
>
> The .NET Core team solves the string problem by providing callbacks. For
> example, their new API that they added to list the runtimes and SDKs on the
> machine: https://github.com/dotnet/runtime/issues/46128.
>
> For these events where we want to allow changing the source, we could pass
> in additional parameters that are callback functions. For example:
>
> struct BA_ONCACHEACQUIRERESOLVING_ARGS
> {
>     PFN_BOOTSTRAPPER_ENGINE_SETLOCALSOURCE pfnSetLocalSource;
>     LPVOID pvContext;
> }
>
> extern "C" typedef HRESULT(WINAPI *PFN_BOOTSTRAPPER_ENGINE_SETLOCALSOURCE)(
>     __in LPCWSTR wzPath,
>     __in LPVOID pvContext
>     );
>
> To keep the binary compatibility goal, it would probably have to instead
> use the ARGS and RESULTS struct paradigm but this is easier to write.
> ____________________________________________________________________
> WiX Toolset Developer Mailing List provided by FireGiant
> http://www.firegiant.com/
>