Vector35 / binaryninja-api

Public API, examples, documentation and issues for Binary Ninja
https://binary.ninja/
MIT License
945 stars 214 forks source link

If condition optimization #6068

Open Zerotistic opened 4 weeks ago

Zerotistic commented 4 weeks ago

Binja shows the following: (example taken from SmsRouterSvc.dll at 0x18002994c - CSmsMessageXmlHelper::GetBroadcastMessageXml)

long result_1 = Windows::Sms::Common::CSmsUtil::GetXmlDocument(nullptr, &var_1b8)
long result = result_1
struct IXMLDOMDocument* rdi = var_1b8

if (result_1 s>= 0) {
    long result_2 = Windows::Sms::Common::CSmsUtil::AddElement(rdi, u"Message", nullptr, &var_1a8)
    result = result_2
    rbx = var_1a8

    if (result_2 s>= 0) {
        long result_3 = Windows::Sms::Common::CSmsUtil::AddElement(rbx, u"MessageType", u"Broadcast", nullptr)
        result = result_3

        if (result_3 s>= 0) {
            long result_4 = Windows::Sms::Common::CSmsUtil::AddElement(rbx, u"SmsDeviceId", r14, nullptr)
            result = result_4

            if (result_4 s>= 0) {
                long result_5 = Windows::Sms::Common::CSmsUtil::AddElement(rbx, u"SimIccId", arg5, nullptr)
                result = result_5

                if (result_5 s>= 0) {
                    uint16_t* bstrString = nullptr
                    long result_12 = (*(*var_1e0 + 0x30))()
                    result = result_12

                    if (result_12 s>= 0) {
                        long result_6 = Windows::Sms::Common::CSmsUtil::AddElement(rbx, u"Text", bstrString, nullptr)
                        result = result_6

 /* if conditions go much further, this is just a sample */

(As a side note, because of the conditions it feels like new result_* are created each time, which just makes it harder to read) When it could be optimized into something like:

long result = Windows::Sms::Common::CSmsUtil::GetXmlDocument(nullptr, xmlDoc);
if (result < 0) {
    return result;
}

IXMLDOMElement* messageElement = nullptr;
result = Windows::Sms::Common::CSmsUtil::AddElement(*xmlDoc, L"Message", nullptr, &messageElement);
if (result < 0) {
    return result;
}

result = Windows::Sms::Common::CSmsUtil::AddElement(messageElement, L"MessageType", L"Broadcast", nullptr);
if (result < 0) {
    return result;
}

result = Windows::Sms::Common::CSmsUtil::AddElement(messageElement, L"SmsDeviceId", smsDeviceId, nullptr);
if (result < 0) {
    return result;
}

result = Windows::Sms::Common::CSmsUtil::AddElement(messageElement, L"SimIccId", simIccId, nullptr);
if (result < 0) {
    return result;
}

BSTR bstrString = nullptr;
result = GetMessageText(&bstrString); 
if (result < 0) {
    return result;
}

More examples from the same binary (SmsRouterSvc.dll): 0x18000fd10 - Windows::Sms::Common::CSmsUtil::GetXmlDocument 0x1800286ec - CSmsMessageXmlHelper::GetStatusMessageXml 0x180048484 - GetDeliverMessageXml (specifically around 0x1800488bd) (^ most references to Windows::Sms::Common::CSmsUtil::GetXmlDocument seems to have the same pattern, and so if conditions)

As #3527 was closed a bit ago, I am opening this one. Below is the dll. SmsRouterSvc.dll.zip This was tested on 4.2.6298-dev

Zerotistic commented 1 week ago

I've got two more last week.

chatroom: 0x004051a0 - handle_send_message_command 0x00405d30 - handle_connection 0x00405660 - handle_logged_in_connection

bagua: 0x0000adc2 - deploy_soldiers 0x00010c2a - adjust_deployment

(As a side note, because of the conditions it feels like new result_* are created each time, which just makes it harder to read)

I noticed something interesting when merging variable, though I'm not entirely sure since I was quite tired at the time (maybe it just refreshed and I thought it cleaned up?). There was a case where the code became notably cleaner after heavily merging variables, but I didn't document where and can't locate it now. This made me question my initial assumption about conditions not being optimized. Perhaps the issue isn't with the optimization itself, but rather with Binja creating new variables too aggressively? We can see it in SmsRouterSvc.dll-Windows::Sms::Common::CSmsUtil::GetXmlDocument: Binja treats each instance of mov rcx, [rbp+arg_18] as a new variable, rather than reusing the same one. Could this aggressive variable creation somehow be affecting condition optimization?

Binaries and bndb: chatroom.zip bagua.zip