Wrong operand size for movb

[jdetter]

Reported by Frederik on the DyninstAPI mailing list:

When decoding the instruction below, we consider -0x1(%ebp) as a 4 byte memory operand instead of a one byte memory operand. However it does appear that we consider the 0x41 as a one byte immediate operand. Is this correct behavior? @wrwilliams @cuviper

 80483e1:       c6 45 ff 41             movb   $0x41,-0x1(%ebp)

Original Message

Hi all,

I use getMemoryWriteOperands() to get the Expression of Operands that write to memory and size() to get the actual size of the write. However, it always returns 4 byte, even if I write a single byte (char) like in the following program.

int main(void) {
   char x = 'A';
   return 0;
}

Even gdb disassembles it to a byte sized write: mov BYTE PTR [ebp-0x1], 0x41. I know that EBP is a 4 byte register, but a 4 byte write would just overwrite the variable next to “x” (at least if layout is not 4 byte aligned). Tell me if I am wrong. I am using a 32-Bit Linux mint (vmware) and Dyninst 9.2.0.

Reproducer

/**
 * reproducer.cpp by Frederik P.
 *
 * Compile with: g++ -std=c++11 -o reproducer ./reproducer.cpp -lpatchAPI -linstructionAPI -ldyninstAPI 
 */
#include <BPatch.h>
#include <BPatch_function.h>
#include <BPatch_flowGraph.h>
#include <Instruction.h>
#include <iostream>

using namespace Dyninst;
using namespace Dyninst::InstructionAPI;

int main(int argc, const char** argv)
{
    BPatch bpatch;
    BPatch_addressSpace *app = bpatch.openBinary("MUTATEE", true);
    BPatch_image *appImage = app->getImage();

    BPatch_Vector<BPatch_function *> functions;
    appImage->findFunction("main", functions);

    BPatch_function* f = functions[0];
    BPatch_flowGraph* fg = f->getCFG();

    std::set<BPatch_basicBlock *> blocks;
    fg->getAllBasicBlocks(blocks);

    for(auto bb_iter = blocks.begin(); bb_iter != blocks.end(); ++bb_iter)
    {
        std::vector<InstructionAPI::Instruction::Ptr> insns;
        (*bb_iter)->getInstructions(insns);

        for(auto ins_iter = insns.begin(); ins_iter != insns.end(); ++ins_iter)
        {
            std::cout << (*ins_iter)->format() << std::endl;

            if((*ins_iter)->writesMemory())
            {

                std::set<Expression::Ptr> memAccessors;
                (*ins_iter)->getMemoryWriteOperands(memAccessors);

                if (memAccessors.size())
                {
                    for(auto it = memAccessors.begin(); it != memAccessors.end(); ++it)
                    {
                        std::cout << "\tmem write (size: " << (*it)->size() << ")" << std::endl;
                    }
                }
            }
        }
    }

    BPatch_binaryEdit *appBin = dynamic_cast<BPatch_binaryEdit *>(app);
    appBin->writeFile("MUTATEE_PATCHED");

    return 0;
}

[wrwilliams]

No, the memory operand should be the same size as the immediate; it's not a sign-extending or zero-extending mov and that means the operand sizes have to match.

[jdetter]

The tree for the memory dereference is like this:

 + DereferenceExpression Size: 1 Format: [EBP + ffffffffffffffff]
  \
   + AddExpression Size: 4 Format: EBP + ffffffffffffffff
    \
     + Register EBP
     + Sign extended Immediate -1 -> ffffffffffffffff

The documentation for getMemoryWriteOperands() says:

Addresses written by this instruction are inserted into memAccessors. The addresses written are in the same form as those returned by getMemoryReadOperands above.

Instruction::getMemoryWriteOperands() returns all of the first children of the operands, which means instead of the DereferenceExpression being returned, it's only child AddExpression is returned, which is size 4 instead of size 1.

@wrwilliams I'm not sure if the documentation is wrong, or the code is wrong or everything is functioning properly.

[wrwilliams]

I think it's the documentation being wrong, then. The effective address itself is 32-bit, the dereference is 8-bit, and we're returning the effective address.

[jdetter]

Me and Bill have decided that we will create a new function with a more intuitive name that implements the correct functionality and deprecate the old function. The documentation will be updated when 9.3.X is released. The old function will remain available until the next major release (10.X.X).

This will be added to the release notes.

[wrwilliams]

Has this gotten implemented without a github update, or is it still open?

[wrwilliams]

Docs do explain this (and have for a long time). Moving to 10.0 for the actual API change.

[mxz297]

Will see if I can reproduce it in Dyninst 10.1

[hainest]

@mxz297 Were you able to reproduce this in 10.1?