IDArlingTeam / IDArling

Collaborative Reverse Engineering plugin for IDA Pro & Hex-Rays
https://idarling.re
GNU General Public License v3.0
660 stars 99 forks source link

IDA Crashes when syncing structures #78

Open ghost opened 5 years ago

ghost commented 5 years ago

2 uses are using IDArling to work on a database.

If user A creates or edits any structure (including renaming fields) then the IDA instance for user B will crash.

From turning the server logging level up to DEBUG it looks like the IDA instance crashes either during or after receiving the DefaultEvent packet that contains the full information of all the structures in user A's database.

Here is a redacted extract from the server log:

[DEBUG] (10.45.3.140:41910) Received packet: DefaultEvent(sname=struc_abc, event_type=struc_member_renamed, offset=0, newname=testing, type=event)
[DEBUG] (10.45.3.123:43426) Sending packet: DefaultEvent(sname=struc_abc, event_type=struc_member_renamed, offset=0, newname=testing, type=event)
[DEBUG] (10.45.3.140:41910) Received packet: DefaultEvent(local_types=[[1, '__n64', '\x1dQÑ\x1b\x02%\x1b\x03$\x1b\x05#\x1b\t"\x1b
<CUT>
[DEBUG] (10.45.3.123:43426) Sending packet: DefaultEvent(local_types=[[1, '__n64', '\x1dQÑ\x1b\x02%\x1b\x03$\x1b\x05#\x1b\t"\x1b
<CUT>

This issue appears to be 100% repeatable across all databases. We are using IDA 7.1 on Ubuntu with the standalone server.

ghost commented 5 years ago

Update: This only appears to happen one way when user A creates or edits structures in their database. If user B makes the change then this is correctly synced to user A's database and no crash occurs.

patateqbool commented 5 years ago

Hi sday15, Thanks for the bug report, I'll push a patch during the day, let me know if it's fix your problem please

patateqbool commented 5 years ago

Hi @sday15, I've tried to fix the bug, unfortunately LocalTypes and user-defined Struct/Enum conflict... I'm still working on a solution for this issue.

NyaMisty commented 5 years ago

We can try to unhook when one of my them occurred?

patateqbool notifications@github.com于2018年11月19日 周一15:48写道:

Hi @sday15 https://github.com/sday15, I've tried to fix the bug, unfortunately LocalTypes and user-defined Struct/Enum conflict... I'm still working on a solution for this issue.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/IDArlingTeam/IDArling/issues/78#issuecomment-439800075, or mute the thread https://github.com/notifications/unsubscribe-auth/AFGMrzMQxIXGbJUtjhFVd8z-ho7Wxcliks5uwmI7gaJpZM4YmCzx .

patateqbool commented 5 years ago

Sorry, I'm not sure to understand the sentence correctly, but here is my answer:

When creating a struct/enum in struc/enum view, first a LocalTypesChanged event is sent, then a Struct/enum related event is sent. For the moment, my only idea is to "queue" the LocalTypesChanged event, wait for the next event, if the next event is a Struct/enum related event, drop the LocalTypesChanged event, else send LocalTypesChanged event then the next event.

NyaMisty commented 5 years ago

Yeah, as the Local Types part was first implemented by me, I noticed this before, and it’s in fact the source of lots of struct issues. I mean that we can unhook the Struct related hooks and re-enable it afterwards. Or we can consider simply remove the struct hooks as it’s connected with local types internally?

patateqbool notifications@github.com于2018年11月20日 周二22:01写道:

Sorry, I'm not sure to understand the sentence correctly, but here is my answer:

When creating a struct/enum in struc/enum view, first a LocalTypesChanged event is sent, then a Struct/enum related event is sent. For the moment, my only idea is to "queue" the LocalTypesChanged event, wait for the next event, if the next event is a Struct/enum related event, drop the LocalTypesChanged event, else send LocalTypesChanged event then the next event.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/IDArlingTeam/IDArling/issues/78#issuecomment-440283406, or mute the thread https://github.com/notifications/unsubscribe-auth/AFGMr5U6vgXeTq3P_6bYmgBXHEcjDtiDks5uxAtKgaJpZM4YmCzx .

patateqbool commented 5 years ago

Currently, we can't rely on our implementation of LocalTypes synchronization because we delete the structure each time a member is added in it. It's a problem because it breaks all the Structure Offset dependency used in the assembly view.

NyaMisty commented 5 years ago

Maybe we can ask HexRays Team for some advice? They have access to the kernel source so they can tell us how to hook correctly.

patateqbool notifications@github.com于2018年11月21日 周三01:24写道:

Currently, we can't rely on our implementation of LocalTypes synchronization because we delete the structure each time a member is added in it. It's a problem because it breaks all the Structure Offset dependency used in the assembly view.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/IDArlingTeam/IDArling/issues/78#issuecomment-440358336, or mute the thread https://github.com/notifications/unsubscribe-auth/AFGMr1E07X5sKo1PKthWgYGyEIWsxJi7ks5uxDrigaJpZM4YmCzx .

patateqbool commented 5 years ago

I agree, I'm going to push a fix that disables localTypes tonight until we have a stable fix to integrate them.

ghost commented 5 years ago

disabling localTypes appears to have fixed the crashing issue.

patateqbool commented 5 years ago

Good to know, I am always looking for a solution to properly integrate localTypes. Thanks for the feedback :)

ghost commented 5 years ago

It looks like applying structures to a memory address in the "IDA View", using Alt-Q does not appear to be synced properly between users anymore. Structures that were created before this change do sync, but any that were created after the change (even though they sync correctly) don't get applied.

patateqbool commented 5 years ago

I guess this manipulation calls LocalTypesChangedEvent, which is now commented due to the previous bug. I confirm the hypothesis tonight and try to find a solution in the coming days. Thanks for the report @sday15