Allow suppression of exceptions with strict: false #55

woodruffw · 2016-07-29T22:17:37Z

Resolves the problem of valid discrepancies between individual Mach-Os in a fat
file causing exceptions during modifications.

ref #50
cc @UniqMartin

woodruffw · 2016-07-29T22:29:53Z

Once this is in, I'll push a release and open a brew PR to update the vendored version/calls to modification methods.

UniqMartin · 2016-07-30T13:45:56Z

I think we might have a misunderstanding what the strict vs. non-strict behavior should be. Or maybe I'm misreading your code. I'm assuming a fat file with more than one Mach-O slice in the following, but thin files (regular Mach-O files) can be treated as if they were fat files with a single slice. Here's what I had in mind:

If the change succeeds for all slices: success in both strict and non-strict mode.
If the change succeeds in some slices, but fails in others: failure in strict mode, but success in non-strict mode. (👈 This is the change I intended.)
If the change fails for all slices: failure in both strict and non-strict mode.

From reading the diff in this PR I got the impression you're also treating case (3) as success in non-strict mode, which is not something I'm willing to allow, as this would open the floodgates and will silently ignore too many problems.

I think the easiest way to achieve what I had in mind (aside from adding options = {} everywhere) would be to modify the methods in FatFile such that they catch exceptions for individual slices, count how many slices were modified successfully, and then re-raise or swallow the errors in accordance with the above list. Does this make sense? (I'm happy to add more clarifying words and/or code snippets.)

Once this is in, I'll push a release and open a brew PR to update the vendored version/calls to modification methods.

It's not strictly necessary, but would be nice to have #49 in the release, too. Otherwise: Agreed!

woodruffw · 2016-07-30T16:17:08Z

From reading the diff in this PR I got the impression you're also treating case (3) as success in non-strict mode, which is not something I'm willing to allow, as this would open the floodgates and will silently ignore too many problems.

You're reading correctly, setting strict: false will currently let all slices fail silently, and this probably isn't good behavior.

I think the easiest way to achieve what I had in mind (aside from adding options = {} everywhere) would be to modify the methods in FatFile such that they catch exceptions for individual slices, count how many slices were modified successfully, and then re-raise or swallow the errors in accordance with the above list.

This makes sense, although I don't generally like using exceptions as control flow. I think it might be easier (and ~~faster~~ more efficient) to accomplish the same thing with something like this at the end of each FatFile modification method:

[...]

successful = machos.count { |m| m.dylib_id == new_id }
raise AnErrorExpressiveOfCompleteFailure if successful == 0

synchronize_raw_data

Where AnErrorExpressiveOfCompleteFailure is a new exception that distinguishes single-point failures from a complete failure to perform the modification on any slice. Thoughts?

It's not strictly necessary, but would be nice to have #49 in the release, too.

Sounds good to me!

woodruffw · 2016-08-04T21:29:57Z

@UniqMartin Any thoughts on my counterproposal for the case when all slices fail? I'd like to push this PR along to keep us on track with the schedule 😄

UniqMartin · 2016-08-05T12:52:33Z

Apologies for the delay! Since we're on the same page regarding the desired behavior, please next time just move ahead and make the necessary adjustments and implement them how you think it should be done, unless doing so would mean an excessive amount of work. It's always easier to discuss actual code and it's quite possible that you'll either notice design flaws in your idea while implementing it or the finished code turns out being superior to what I had in mind.

In any case and unless you already have done so, please use the time until you hear back from me to construct tests that check for the desired behavior, e.g. for this PR fat binaries with inconsistent Mach-O slices and test code that works with them.

This makes sense, although I don't generally like using exceptions as control flow.

In general, I tend to agree here. The idea that was backing this suggestion would be that this would allow for richer error handling, where the exception raised in the top-level modification method that ensures the modification could be applied to any/all slices could more easily point out what went wrong and which Mach-O slice was responsible for this.

I think it might be easier (and ~~faster~~ more efficient) to accomplish the same thing with something like this at the end of each FatFile modification method: […] Where AnErrorExpressiveOfCompleteFailure is a new exception that distinguishes single-point failures from a complete failure to perform the modification on any slice. Thoughts?

I think this idea works fairly well for the presented case of LC_ID_DYLIB but will quickly become more complex for the other load commands and unnecessarily so, since the called method that operates on a single Mach-O slice already does all the necessary checks.

woodruffw · 2016-08-05T14:29:51Z

I think this idea works fairly well for the presented case of LC_ID_DYLIB but will quickly become more complex for the other load commands and unnecessarily so, since the called method that operates on a single Mach-O slice already does all the necessary checks.

Yeah, that's true. Doing this by filtering exceptions sounds good to me with that in mind.

Do you have a particular structure in mind? Catching exceptions and counting them with an accumulator is what immediately comes to mind, but that doesn't feel very ruby-esque.

In any case and unless you already have done so, please use the time until you hear back from me to construct tests that check for the desired behavior, e.g. for this PR fat binaries with inconsistent Mach-O slices and test code that works with them.

I'll get on this. I think there's a cctools utility for smashing binaries together (i.e., the opposite of lipo(1)), so we can probably leverage that to make some fat binaries with inconsistent slices.

woodruffw · 2016-08-05T14:32:56Z

i.e., the opposite of lipo(1)

Never mind, looks like lipo(1) does this directly with -create 😄

UniqMartin · 2016-08-05T14:56:45Z

Do you have a particular structure in mind? Catching exceptions and counting them with an accumulator is what immediately comes to mind, but that doesn't feel very ruby-esque.

Roughly, this is what I had in mind:

errors = []
machos.each_with_index do |macho, index|
  begin
    macho.‹operation›
  rescue AppropriateBaseClass => e
    errors << [index, e]
  end
end

strict = options.fetch(:strict, false)
failed = strict ? !errors.empty? : errors.size == machos.size
if failed
  macho_index, cause = errors.first
  raise MachOSliceModificationError.new(strict, macho_index, cause)
end

Here, AppropriateBaseClass could potentially be ArgumentError and DylibUnknownError and similar exception classes could become children of ArgumentError. (I haven't fully fleshed out this thought, but it would be nice to have a distinction between exception classes that are due to problems with the binary that we're reading and exceptions due to inappropriate use of our API, because I only want the latter to be rescued and possibly ignored if in non-strict mode.)

It obviously doesn't make sense to repeat all this in every one of the methods, but the logic could be easily encapsulated, such that, e.g., FatFile#delete_rpath could look something like:

def delete_rpath(path, options = {})
  each_machos_with_check(options) do |macho|
    macho.delete_rpath(path)
  end
  synchronize_raw_data
end

Of course all those newly invented names are up for debate (I didn't think too long about them and as we all know naming things is hard).

woodruffw · 2016-08-05T15:47:35Z

Here, AppropriateBaseClass could potentially be ArgumentError and DylibUnknownError and similar exception classes could become children of ArgumentError.

Since we already have a (flat) MachOError hierarchy going on, I could make exceptions like DylibUnknownError all subclasses of MachOModificationError or something similar. This would allow us to be sufficiently generic while still distinguishing between API validation and modification failure.

each_machos_with_check(options)

I think something like this would be ideal (perhaps just each_macho, given that the optional options parameter will be documented?)

UniqMartin · 2016-08-05T16:17:03Z

Sounds all good to me! Let's get this idea implemented and then we can discuss the finer details with actual code in front of us. (I doubt we'll have to overthrow the entire design again.)

woodruffw · 2016-08-05T21:50:40Z

I could make exceptions like DylibUnknownError all subclasses of MachOModificationError or something similar.

After thinking about this, I think that it makes more sense to just rescue the toplevel MachOError exception - that way we'll catch mistakes that "succeed" are only exposed when the header/load commands are re-parsed.

UniqMartin · 2016-08-05T22:03:43Z

After thinking about this, I think that it makes more sense to just rescue the toplevel MachOError exception - that way we'll catch mistakes that "succeed" are only exposed when the header/load commands are re-parsed.

(I have to admit that I failed to fully parse this comment, thus forgive me if my comment is misguided.) This sounds like a dangerous path to take. We definitely don't want to rescue and silence all exceptions if they only occur for a single Mach-O slice out of possibly many slices.

An example: One slice cannot be modified because there's insufficient space in the Mach-O header padding, while all other slices have sufficient padding and thus the update succeeds there. This should be causing an error even in non-strict mode.

woodruffw · 2016-08-05T22:06:58Z

(I have to admit that I failed to fully parse this comment, thus forgive me if my comment is misguided.) This sounds like a dangerous path to take. We definitely don't want to rescue and silence all exceptions if they only occur for a single Mach-O slice out of possibly many slices.

You understood it correctly! I wasn't thinking, we definitely don't want to silence those types of errors.

UniqMartin · 2016-08-05T22:08:05Z

lib/macho/exceptions.rb

+      strictness = strict ? "strict" : "nonstrict"
+      inner_error = "#{cause.class}: #{cause}"
+
+      super "#{inner_error} during #{strictness} modification at slice #{index}"


“[…] modification of slice […]”?

woodruffw · 2016-08-05T22:35:09Z

Using an encapsulating exception like MachOSliceModificationError breaks the isomorphism between MachOFile and FatFile, in the sense that the two no longer throw the same exception:

macho.dylib_id = "x" * 4096 # HeaderPadError
fat.dylib_id = "x" * 4096 # MachOSliceModificationError

That's not a dealbreaker for me (since both are children of MachOModificationError), but it might be a source of confusion. Unfortunately, I don't really have an alternative that preserves the valuable information we get from being in the context of FatFile (i.e., index).

Besides that (very minor) nit, what do you think of renaming MachOSliceModificationError to FatSliceModificationError? I think that the latter more accurately reflects the semantics of the exception (a failure to modify a slice of a fat file, not a single-arch Mach-O).

UniqMartin · 2016-08-06T09:25:09Z

Using an encapsulating exception like MachOSliceModificationError breaks the isomorphism between MachOFile and FatFile, in the sense that the two no longer throw the same exception:
macho.dylib_id = "x" * 4096 # HeaderPadError
fat.dylib_id = "x" * 4096 # MachOSliceModificationError

I can relate to this concern. Though you managed to pick an example that isn't related to the problem. 😉 I thought we had agreed that a HeaderPadError should always be a fatal condition, no matter whether strict or not and no matter if fat or thin binary. MachOSliceModificationError should only be wrapping exceptions that can be non-fatal under certain circumstances.

That's not a dealbreaker for me (since both are children of MachOModificationError), but it might be a source of confusion. Unfortunately, I don't really have an alternative that preserves the valuable information we get from being in the context of FatFile (i.e., index).

We could also equip MachOModificationError with a macho_slice attribute that defaults to nil and gets set in FatFile#each_macho after rescuing the exception and before re-raising it. MachOModificationError could contain the necessary logic to adjust the message depending on whether macho_slice is set. (And I guess we'll need a strictness attribute, too.)

Now that this popped into my head, I think this is a superior approach to the error handling (or at least to how the exceptions are organized and re-raised). What do you think?

Besides that (very minor) nit, what do you think of renaming MachOSliceModificationError to FatSliceModificationError? I think that the latter more accurately reflects the semantics of the exception (a failure to modify a slice of a fat file, not a single-arch Mach-O).

I guess this question is somewhat irrelevant, if you agree on the above. But my thinking was that MachOSlice (because it is only a slice of possibly many slices) reflects the information that it is part of a fat file. In contrast, FatSlice sounds to me like there is a fat slice somewhere, but there's actually none and the slice itself is thin (and part of a single fat file).

UniqMartin · 2016-08-06T09:30:49Z

lib/macho/exceptions.rb

@@ -3,6 +3,10 @@ module MachO
  class MachOError < RuntimeError
  end

+  # A generic Mach-O error during file modification.
+  class MachOModificationError < MachOError


Let's be super-explicit here. I think this should be something like “A generic Mach-O error during file modification, that is non-fatal and might be suppressed if operating in non-strict mode.” (I'm not particularly happy with this wording, but I hope the message gets across. Please feel free to phrase this better!)

We could also consider a name different from MachOModificationError that better reflects the nature of these exceptions.

We could also consider a name different from MachOModificationError that better reflects the nature of these exceptions.

This might get us uncomfortably close to checked and unchecked exceptions a la Java, but how about NonFatalModificationError (and FatalModificationError for things like HeaderPadError)? (Update: Got rid of the Mach-O prefixes...)

Agreed on the documentation 👍

I'm a little confused about how we're using "fatal" here, actually: do we mean "fatal" as in "halts the program" or as in "would do something very bad [to the Mach-O] if it were allowed to continue"?

Sorry for the confusion! I'm not entirely happy with the “fatal” term either but couldn't think of a better term. In this context I was using it to distinguish between modification errors that should always be raised (header pad error being one example) and modification errors that could be absorbed by our front-end API (e.g. FatFile#delete_rpath) in non-strict mode (e.g. RPATH deletion succeeds on one Mach-O slice but fails on another).

No worries! I was also confusing myself my reusing the term in the exceptions. How do you feel about CheckedModificationError and UncheckedModificationError instead? That gets us away from the connotation of program halting and more accurately conveys the fact that we're checking and filtering subclasses of the particular error.

UniqMartin · 2016-08-06T09:40:54Z

lib/macho/macho_file.rb

+      if old_lc.nil?
+        return unless options.fetch(:strict, true)
+        raise RpathUnknownError.new(old_path)
+      end


This change needs to be reverted. The modification methods in MachOFile need to always raise an exception, independently of options (because options currently only matters if multiple Mach-O slices are involved).

You brought back the original checks further up in this method but still left this new code intact?

woodruffw · 2016-08-06T15:22:17Z

We could also equip MachOModificationError with a macho_slice attribute that defaults to nil and gets set in FatFile#each_macho after rescuing the exception and before re-raising it. MachOModificationError could contain the necessary logic to adjust the message depending on whether macho_slice is set. (And I guess we'll need a strictness attribute, too.)

Could you describe the structure you have in mind? Sorry, I'm having a little trouble visualizing it 😅

UniqMartin · 2016-08-06T21:51:59Z

lib/macho/exceptions.rb

@@ -3,6 +3,14 @@ module MachO
  class MachOError < RuntimeError
  end

+  # A generic and non-fatal Mach-O error during file modification.


I think we can drop generic from the description. It doesn't seem to add any value.

UniqMartin · 2016-08-07T05:58:24Z

Pulling this out from #55 (comment) so it's not completely hidden (and lost):

No worries! I was also confusing myself my reusing the term in the exceptions. How do you feel about CheckedModificationError and UncheckedModificationError instead? That gets us away from the connotation of program halting and more accurately conveys the fact that we're checking and filtering subclasses of the particular error.

That's a nice suggestion; I think that's terminology we could borrow. I went looking for something similar in the Ruby world, but I didn't find anything.

Another idea I just came up with is to use ModificationError and ModificationWarning. The only problem I have with this is that having something named *Warning in a hierarchy of exception classes feels a bit weird. What we're effectively doing with the strict behavior for fat binaries is to promote warnings to errors. (Maybe RecoverableModificationError is better?) In any case, and just to be sure we're on the same page, here's the exception hierarchy I'm envisioning:

-- MachOError
   |-- (various other exceptions)
   \-- ModificationError
       |-- RecoverableModificationError (ModificationWarning)
       |   |-- DylibUnknownError
       |   |-- DylibIdMissingError
       |   |-- RpathUnknownError
       |   \-- RpathExistsError
       |-- OffsetInsertionError
       \-- HeaderPadError

Does this look reasonable?

Could you describe the structure you have in mind? Sorry, I'm having a little trouble visualizing it 😅

Will do. Give me a few moments to prepare some demo code.

UniqMartin · 2016-08-07T07:51:44Z

Could you describe the structure you have in mind? Sorry, I'm having a little trouble visualizing it 😅

Code (changes on top of this branch): UniqMartin@cfa5e91 in my strictness branch

Preparations:

$ cp test/bin/i386/hello.bin hello-i386
$ cp test/bin/x86_64/hello.bin hello-x86_64
$ lipo -create -output hello-consistent hello-i386 hello-x86_64
$ install_name_tool -add_rpath /mesa/special hello-x86_64
$ lipo -create -output hello-inconsistent hello-i386 hello-x86_64
$ ruby -Ilib -rmacho -e 'puts MachO.open(ARGV.shift).machos.map(&:rpaths).inspect' hello-consistent
[["made_up_path"], ["made_up_path"]]
$ ruby -Ilib -rmacho -e 'puts MachO.open(ARGV.shift).machos.map(&:rpaths).inspect' hello-inconsistent
[["made_up_path"], ["made_up_path", "/mesa/special"]]

Tests (the important difference and the point of this PR is the treatment of hello-inconsistent):

$ # Strict mode:
$ add-rpath() { ruby -Ilib -rmacho -e 'MachO.open(ARGV.shift).add_rpath(*ARGV, :strict => true) rescue puts $!.inspect' "$@"; }
$ add-rpath hello-inconsistent /mesa/special
#<MachO::RpathExistsError: While modifying Mach-O slice 1: /mesa/special already exists>

$ # … no change for consistent fat binaries:
$ add-rpath hello-consistent made_up_path
#<MachO::RpathExistsError: While modifying Mach-O slice 0: made_up_path already exists>
$ add-rpath hello-consistent /mesa/special

$ # … no change for non-fat binaries:
$ add-rpath hello-i386 made_up_path
#<MachO::RpathExistsError: made_up_path already exists>
$ add-rpath hello-i386 /mesa/special

$ # Non-strict mode:
$ add-rpath() { ruby -Ilib -rmacho -e 'MachO.open(ARGV.shift).add_rpath(*ARGV, :strict => false) rescue puts $!.inspect' "$@"; }
$ add-rpath hello-inconsistent /mesa/special

$ # … no change for consistent fat binaries:
$ add-rpath hello-consistent made_up_path
#<MachO::RpathExistsError: While modifying Mach-O slice 0: made_up_path already exists>
$ add-rpath hello-consistent /mesa/special

$ # … no change for non-fat binaries:
$ add-rpath hello-i386 made_up_path
#<MachO::RpathExistsError: made_up_path already exists>
$ add-rpath hello-i386 /mesa/special

woodruffw · 2016-08-07T13:25:12Z

Another idea I just came up with is to use ModificationError and ModificationWarning. The only problem I have with this is that having something named *Warning in a hierarchy of exception classes feels a bit weird. What we're effectively doing with the strict behavior for fat binaries is to promote warnings to errors. (Maybe RecoverableModificationError is better?)

Yeah, I think ModificationWarning will ultimately become confusing (since single-arch Mach-Os will be raising subclasses of it and not filtering them before getting to the user). RecoverableModificationError gets us to the same meaning as CheckedModificationError, with the added benefit of not being mutually exclusive (like CheckedModificationError is with UncheckedModificationError) 👍

In any case, and just to be sure we're on the same page, here's the exception hierarchy I'm envisioning

Yep!

woodruffw · 2016-08-07T13:42:11Z

Code (changes on top of this branch): UniqMartin/ruby-macho@cfa5e91 in my strictness branch

This looks great! Re-raising the exception immediately with the added context feels much more natural to me.

I cherry-picked your commit right on top of this branch - do you know how to preserve authorship (if possible) during a squash?

UniqMartin · 2016-08-07T14:04:13Z

I cherry-picked your commit right on top of this branch - do you know how to preserve authorship (if possible) during a squash?

I guess there must be a more elegant solution, but this works:

$ git rebase -i HEAD~2 # squash as usual
$ git commit --no-edit --amend --author="$(git show -s --format='%an <%ae>' origin/lc-modification-strictness)" # steal authorship information from commit authored by me

…tions. Resolves the problem of valid discrepancies between individual Mach-Os in a fat file causing exceptions during modifications. Distinguish between recoverable and unrecoverable modification errors. Closes #50.

UniqMartin · 2016-08-07T14:30:08Z

I feel a bit bad stealing your commit credit as you've done the bulk of the work, though. 😳

woodruffw · 2016-08-07T14:36:37Z

I feel a bit bad stealing your commit credit as you've done the bulk of the work, though.

Don't worry about it; although we could probably do something like --author="William Woodruff <william@tuffbizz.com> and Martin Afanasjew <martin@afanasjew.de>". I'm not sure if GitHub would like that, though.

Either way, I still show up as the PR's author and committer, so I'm fine with it 😄

UniqMartin · 2016-08-07T15:02:07Z

Either way, I still show up as the PR's author and committer, so I'm fine with it 😄

Then please go ahead and squash-merge this yourself, assuming you're happy with the PR. (I think it's ready.) Otherwise if I do this myself, you'll be robbed of your committer credit, too.

woodruffw · 2016-08-07T15:21:06Z

Done! 🎉

UniqMartin · 2016-08-07T15:26:09Z

Awesome! 🎉 Thanks for surviving through my reviews and constant complaints. 😂

woodruffw added the bug label Jul 29, 2016

woodruffw added this to the Release 1.0.0 milestone Jul 29, 2016

woodruffw force-pushed the lc-modification-strictness branch 2 times, most recently from f7dae25 to 6882bc5 Compare July 31, 2016 21:05

woodruffw force-pushed the lc-modification-strictness branch from 6882bc5 to 84a9cfe Compare August 5, 2016 21:59

UniqMartin reviewed Aug 5, 2016
View reviewed changes

woodruffw force-pushed the lc-modification-strictness branch 2 times, most recently from 098fb05 to 35baf19 Compare August 5, 2016 22:25

UniqMartin reviewed Aug 6, 2016
View reviewed changes

woodruffw force-pushed the lc-modification-strictness branch from 35baf19 to 444c333 Compare August 6, 2016 14:47

UniqMartin reviewed Aug 6, 2016
View reviewed changes

woodruffw force-pushed the lc-modification-strictness branch from 444c333 to 7fb42da Compare August 6, 2016 22:04

woodruffw force-pushed the lc-modification-strictness branch from d16c04e to d395c05 Compare August 7, 2016 14:14

woodruffw merged commit 0d3741c into master Aug 7, 2016

woodruffw deleted the lc-modification-strictness branch August 7, 2016 15:21

This was referenced Aug 7, 2016

vendor ruby-macho 0.2.5 Homebrew/brew#656

Closed

Expanding Makefile/build system to create inconsistent fat binaries #60

Closed

lock bot added the outdated label Jan 1, 2020

lock bot locked as resolved and limited conversation to collaborators Jan 1, 2020

Allow suppression of exceptions with strict: false #55

Allow suppression of exceptions with strict: false #55

Conversation

woodruffw commented Jul 29, 2016

woodruffw commented Jul 29, 2016

UniqMartin commented Jul 30, 2016

woodruffw commented Jul 30, 2016 • edited

woodruffw commented Aug 4, 2016

UniqMartin commented Aug 5, 2016

woodruffw commented Aug 5, 2016

woodruffw commented Aug 5, 2016

UniqMartin commented Aug 5, 2016

woodruffw commented Aug 5, 2016

UniqMartin commented Aug 5, 2016

woodruffw commented Aug 5, 2016

UniqMartin commented Aug 5, 2016

woodruffw commented Aug 5, 2016

Choose a reason for hiding this comment

woodruffw commented Aug 5, 2016 • edited

UniqMartin commented Aug 6, 2016

Choose a reason for hiding this comment

woodruffw Aug 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodruffw commented Aug 6, 2016

Choose a reason for hiding this comment

UniqMartin commented Aug 7, 2016

UniqMartin commented Aug 7, 2016

woodruffw commented Aug 7, 2016

woodruffw commented Aug 7, 2016

UniqMartin commented Aug 7, 2016

UniqMartin commented Aug 7, 2016

woodruffw commented Aug 7, 2016

UniqMartin commented Aug 7, 2016

woodruffw commented Aug 7, 2016

UniqMartin commented Aug 7, 2016

woodruffw commented Jul 30, 2016 •

edited

woodruffw commented Aug 5, 2016 •

edited

woodruffw Aug 6, 2016 •

edited