google / binexport

Export disassemblies into Protocol Buffers
Apache License 2.0
1.05k stars 204 forks source link

Issue with BinExport for Binary Ninja #109

Closed ghost closed 1 year ago

ghost commented 1 year ago

I just built BinExport for Binary Ninja (from latest commit as of writing) and now Binja crashes when launching the app.

I'm on macOS 12.6.3 and using the latest development build of Binja

psifertex commented 1 year ago

Looks like it's working fine, just a bit fragile -- this is basically a dupe of #108

To build with the latest development versions of BN you have to manually update the hard-coded hashes in the cmake files.

The latest dev git tag as of right now should be b57e34087f3c2e9bc76b841720a728bb54fbbf4d which should go here: https://github.com/google/binexport/blob/main/cmake/BinExportDeps.cmake#L97

Next, you'll want to re-generate the headers (which requires clang-format):

$ cd third_party/binaryninja_api/
$ ./regenerate-api-stubs.sh /Users/jwiens/vector35/binaryninja/api #point to wherever your have a copy of the BN API repo checked out

Finally, you should be able to build:

$ mkdir build && cd build
$ cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Release "-DCMAKE_INSTALL_PREFIX=$PWD" -DBINEXPORT_ENABLE_IDAPRO=OFF -DBINEXPORT_ENABLE_BINARYNINJA=ON
...
$ cmake --build . --config Release
...
$ cd $BN_PLUGIN_FOLDER #this is platform dependent, use "plugins / open plugin folder" in the UI to find the right path
$ ln -s ~/dev/binexport/build_mac/binaryninja/binexport12_binaryninja.dylib .

After that it worked for me on build 4097.

Screenshot 2023-02-28 at 12 40 04 PM

cblichmann commented 1 year ago

Note that for organizational reasons, I can't easily provide binaries for both stable and dev (and dev changes too quickly). So I mostly test with stable.

psifertex commented 1 year ago

@0x8ff did my above steps work for you or are you still having any issues?

ghost commented 1 year ago

@psifertex i wrote a shell script based on the commands you told me to run: https://gist.github.com/0x8ff/564dd238b5f4842da3ae19589b1ad985

i get this at each build: (and yes sed does change the git hash in BinExportDeps.cmake correctly)

wtf

ghost commented 1 year ago

Even doing the commands manually exactly as you listed, I get the same issue

psifertex commented 1 year ago

You're sure you ran the generator script against a checked out development copy of the repo at the right commit hash? I ran into that issue before I ran the generator if I recall correctly.

psifertex commented 1 year ago

Actually, let me make a few mods to that script to have it validate the hash with the installed copy of binary ninja and check out that version of the repo as well. That should fully automate it 🤞

psifertex commented 1 year ago

Ahh, yeah, it looks like you also have to define BINEXPORT_BINARYNINJA_CHANNEL to anything besides STABLE which it defaults to. My previous hacky changes above had mucked with the cmake locally in a way that it wasn't required anymore and I forgot to update the line above, my appologies.

I've updated your gist here with that and a few other changes (the main one is it will match whatever the current version of BN you have installed in the default location, so it'll easily work even if you're not on the exact latest dev). Let me know if it works better for you:

https://gist.github.com/psifertex/31d9bc3167eca91e466ebaae4382521c

ghost commented 1 year ago

Thanks, this plugin now works for me :)

psifertex commented 1 year ago

Great! Thanks for the idea of the script. I imagine you can close the issue now?

Maybe @cblichmann can integrate that script into the repo or documentation but you can probably close this issue then. I don't know about your contributions, but I hereby release any changes I made into the public domain for anyone to do what they want with.

cblichmann commented 1 year ago

Thanks, @psifertex! I think I'll turn this into an update-binary-ninja-dep.sh and use this myself when updating.

psifertex commented 1 year ago

It might be worth dropping the distinction between dev or stable as well as I think it would simplify the process some? Basically, just pull the API hash out of whatever version of BN you are building for and go from there. Only hurdle left would be to simplify the header regeneration and then it could be much smoother all around with minimal dependencies.

cblichmann commented 1 year ago

It might be worth dropping the distinction between dev or stable as well as I think it would simplify the process some? Hmm, the idea is to have CMake download dependencies automatically during configure time, so some switch is kind of necessaary. But like you mentioned earlier in this thread, the CMake build should just auto-regenerate the bindings if possible.

psifertex commented 1 year ago

FYI -- I just fixed a bug with that script (I wasn't doing a fetch on the BN repo if it already existed). Same gist url as before, but if @cblichmann already was building something on it, just a heads up.

I think it's also safe to close this at this point too since it's fairly well resolved even if just by that script?