Skip to content

iOS: add CodePushBinaryDiffPatcher to apply bsdiff patches - #59

Merged
ofalvai merged 1 commit into
ios-diff-manifest-parsingfrom
ios-binary-diff-patcher
Sep 22, 2026
Merged

ofalvai merged 1 commit into
ios-diff-manifest-parsingfrom
ios-binary-diff-patcher

Conversation

@ofalvai

@ofalvai ofalvai commented Sep 9, 2026 •

Copy link
Copy Markdown
Collaborator

Context

iOS counterpart of #42. This is the diff patching logic that #48 is going to integrate into the main package install flow.

What

CodePushBinaryDiffPatcher takes a manifest (from #58), the current update folder, the unzipped update folder, and the folder where the new update files should live after applying the patches.

@ofalvai
ofalvai added this pull request to stack #57 September 9, 2026 06:46
@ofalvai ofalvai changed the title ios binary diff patcher iOS: add CodePushBinaryDiffPatcher to apply bsdiff patches Sep 9, 2026
@ofalvai
ofalvai force-pushed the ios-binary-diff-patcher branch from 7912ae8 to ca765df Compare September 9, 2026 15:37
Copilot AI balanced review requested due to automatic review settings September 9, 2026 15:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new patcher currently emits misleading hash-mismatch errors when hashing fails (e.g., unreadable/missing files), obscuring the real I/O failure cause.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds an iOS-native binary diff patcher to apply bsdiff patches described by CodePushDiffManifest, aligning iOS with the existing Android diff/patch flow and hardening path handling via resolvePath(_:withinFolder:).

Changes:

  • Introduces CodePushBinaryDiffPatcher (Obj-C) to validate hashes, resolve manifest-supplied paths safely, and apply bsdiff patches via bspatch_bridge.
  • Adds Swift XCTest coverage for happy-path patching, hash mismatches, unsupported algorithms, path traversal, and symlink-escape attempts.
  • Wires the new source + tests into the Xcode project and test bridging header.
File summaries
File Description
ios/CodePushTests/CodePushTests-Bridging-Header.h Exposes the new Obj-C patcher to Swift tests.
ios/CodePushTests/CodePushBinaryDiffPatcherTests.swift Adds test coverage for binary patch application and path/symlink safety.
ios/CodePush/CodePushBinaryDiffPatcher.m Implements patch application, path resolution, and hash verification.
ios/CodePush/CodePushBinaryDiffPatcher.h Declares the patcher API and Swift import name.
ios/CodePush.xcodeproj/project.pbxproj Registers new sources/headers and the new test file in build phases.
Review details

Suppressed comments (1)

ios/CodePush/CodePushBinaryDiffPatcher.m:98

  • If hashing the newly patched file fails, newFileHash can be nil and the code reports a confusing "targetHash mismatch ... got (null)" instead of the actual hashing/read error. Handling the nil case separately provides a clearer failure reason.
        NSString *newFileHash = CodePushSha256HexForFile(newFile, &hashError);
        if (!newFileHash || ![newFileHash isEqualToString:entry.targetHash]) {
            if (error) *error = patchApplyError(relativePath, [NSString stringWithFormat:@"targetHash mismatch: expected %@, got %@", entry.targetHash, newFileHash]);
            return NO;
  • Files reviewed: 5/5 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +64 to +69
NSError *hashError = nil;
NSString *oldFileHash = CodePushSha256HexForFile(oldFile, &hashError);
if (!oldFileHash || ![oldFileHash isEqualToString:entry.baseHash]) {
if (error) *error = patchApplyError(relativePath, [NSString stringWithFormat:@"baseHash mismatch: expected %@, got %@", entry.baseHash, oldFileHash]);
return NO;
}

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.

Fixed

@ofalvai
ofalvai force-pushed the ios-binary-diff-patcher branch from ca765df to 1f18747 Compare September 14, 2026 14:07
@ofalvai
ofalvai marked this pull request as ready for review September 14, 2026 14:12
@ofalvai
ofalvai force-pushed the ios-binary-diff-patcher branch from 1f18747 to 4cd11f4 Compare September 14, 2026 14:21

@miklosboros miklosboros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two notes from reviewing this against ios-diff-manifest-parsing and Android's BinaryDiffPatcher.kt. The patcher itself reads well — hash before and after, the two-pass validation, and the bridge's own cleanup mean a failed patch can't leave a wrong-but-plausible file behind, and the error messages carry more detail than Android's.

Comment on lines +19 to +21
1993F82DF6330426AC5CBC2E /* CodePushBinaryDiffPatcher.m in Sources */ = {isa = PBXBuildFile; fileRef = 9414C770C70E1B1319F24B12 /* CodePushBinaryDiffPatcher.m */; };
F41E2EFAD0D322F7D14D7125 /* CodePushBinaryDiffPatcher.m in Sources */ = {isa = PBXBuildFile; fileRef = 9414C770C70E1B1319F24B12 /* CodePushBinaryDiffPatcher.m */; };
D5BC8B1BF7D2DA03BB888FCC /* CodePushBinaryDiffPatcher.m in Sources */ = {isa = PBXBuildFile; fileRef = 9414C770C70E1B1319F24B12 /* CodePushBinaryDiffPatcher.m */; };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These three entries add CodePushBinaryDiffPatcher.m to CodePush, CodePush-tvOS and CodePushTests — but shared/diffpatch/bspatch_bridge.c (and the hdiffpatch sources) are not in the tvOS target, and the patcher's codepush_bspatch_apply call at CodePushBinaryDiffPatcher.m:89 is its only caller. I parsed both trees to confirm:

BASE (#58)  tvOS: bspatch_bridge.c=no  patcher=no   Sha256=no  UpdateUtils=YES
PR#59       tvOS: bspatch_bridge.c=no  patcher=YES  Sha256=no  UpdateUtils=YES

So _codepush_bspatch_apply becomes unresolved in CodePush-tvOS, and it wasn't before this PR.

Two things keep this from being a build break, and I want to be precise about them rather than overstate it: CodePush-tvOS is a static library, so unresolved symbols are legal in the archive and xcodebuild will not fail on that target. And CocoaPods consumers are unaffected — the podspec compiles shared/diffpatch/*.c and the hdiffpatch sources for tvOS as well. It surfaces at app link time only for someone integrating tvOS manually from the .xcodeproj.

Separately and pre-existing, so not yours to fix here: _CodePushSha256HexForFile is also unresolved in that target — CodePushUpdateUtils.m already referenced it on the base branch with CodePushSha256.m absent. The patcher just becomes a second caller.

Either drop the patcher from CodePush-tvOS, or add bspatch_bridge.c + the hdiffpatch sources (and CodePushSha256.m) to it.

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.

This tvOS target frequently gets flagged, and we should clean it up one day, I just want to better understand what was its original purpose. It's been broken for years, we don't really use it in CI anyway, and it doesn't get distributed to users (Cocoapods is the distribution mechanism). I think it's fine for now.

Comment on lines +27 to +34
static NSString *resolveWithin(NSString *base, NSString *path, NSString *manifestEntry, NSError **error)
{
NSString *resolved = [CodePushDiffManifest resolvePath:path withinFolder:base];
if (resolved == nil) {
if (error) *error = patchApplyError(manifestEntry, @"path escapes expected directory");
}
return resolved;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This maps every nil from resolvePath:withinFolder: to "path escapes expected directory", but #58's resolver returns nil for at least four distinct causes:

Two of those are manifest-triggerable: patchedFiles: {"": {...}}, or a key containing a NUL, both report a path escape for what is only a malformed manifest.

Worth noting this is an iOS-only divergence. Android's resolveWithin uses File.canonicalFile, which doesn't require the path to exist, so a missing currentPackageFolder there surfaces as a genuine read failure out of sha256Hex instead. Same manifest, same condition, very different diagnosis — and "path escapes" is the one that sends whoever reads the log looking for an attack.

Threading the reason out of resolvePath (or at least distinguishing "base folder unusable" and "path not representable" from a real escape) would fix it.

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.

Decided to fix this by adopting the standard ObjC pattern of a NSError* argument. The caller can log this root cause, and ObjC -> Swift interop transforms this pattern into real exceptions.

PR #58 is updated with the change to resolvePath:withinFolder, and this PR with the changes to the callsites.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good choice, thanks!

@ofalvai
ofalvai force-pushed the ios-binary-diff-patcher branch 2 times, most recently from e27524d to 3a07cf9 Compare September 21, 2026 11:49
@ofalvai
ofalvai force-pushed the ios-binary-diff-patcher branch from 3a07cf9 to fc26ab7 Compare September 22, 2026 08:10
@ofalvai
ofalvai merged commit 7d6b4c5 into master Sep 22, 2026
6 checks passed
@ofalvai
ofalvai deleted the ios-binary-diff-patcher branch September 22, 2026 14:40
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.

3 participants