6
\$\begingroup\$

I am reviewing some C code (see extract below) that uses inline asm. I believe there are 4 spec violations, 1 inefficiency and 1 honking big design flaw in this code. While some of the problems I'm seeing apply to both the x86 and x64 sections in that code, this post is focused on x64.

Before I'll be allowed to check in a fix, I must first convince people that there is a problem. And the number of people prepared to review inline asm on that project is limited. Hopefully someone here can provide this.

What the code is trying to do:

This code is intended to be a wrapper that turns a vsscanf call (int vsscanf(const char *buffer, const char *format, va_list arglist)) into a sscanf call (int sscanf(const char *buffer, const char *format [,argument] ...)) on (Windows) platforms that don't have a builtin vsscanf. Unfortunately, there is no defined way to know how big a va_list is, so the existing code takes a rather... unconventional approach, which, as it turns out, doesn't work.

Here are the problems I believe exist in the current code:

  1. As part of the work, this code calls memcpy. However, it looks to me like there isn't enough room reserved on the stack for the call. While memcpy only takes 3 parameters, by spec:

    The caller is responsible for allocating space for parameters to the callee, and must always allocate sufficient space for the 4 register parameters, even if the callee doesn’t have that many parameters.

    And the 4th parameter would be at 0x18(%%rsp), which is being used for something else. If (for some bizarre reason) memcpy decided to use the memory that was supposed to have been allocated for that 4th parameter, bad things would happen, right?

  2. Windows 64bit stacks are supposed to be 16 bit aligned, and this seems to call memcpy with a (potentially) unaligned stack (ie no andq $-16, %%rsp). Problem?

  3. For a brief instant, rsp has a wild value (after lea 0xFFFFFFFFFFFFFFD8(%%rsp, %6), %%esp and before subq %5, %%rsp). If during that (microscopic) interval an interrupt were to occur, the program would probably crash. I'm pretty sure the requirement is that rsp always be valid.

  4. While functions must reserve stack space for 4 parameters, the actual values are passed in registers. I don't believe the caller must duplicate the registers into the stack before making the call. The way this code does before calling memcpy. It won't give incorrect results, but it seems pointless.

  5. Most importantly: As you can see, the function declares a local variable named ret. And it (apparently) believes that however big the va_list is, it must all fit between ptr and &ret, so it allocates enough stack space to hold that much data, and copies everything in between to "the right place" in the stack in preparation for the sscanf call. However, the data is NOT between ptr and &ret, it's just on the other side of ptr. The code only works because the distance between ptr and &ret just happens to be big enough in most cases.

  6. The code modifies the stack, but does not include the appropriate .seh_* directives. Despite the fact that these directives are undocumented in gas, you see them used in gcc output on Windows builds. My understanding is that if the code might be in the stack during an SEH unwind operation, it is expected to use these.

Here's the original code:

int __ms_vsscanf (const char * __restrict__ s, const char * __restrict__ format, va_list arg) {
  int ret;

  __asm__ __volatile__ (

    // allocate stack (esp += frame - arg3 - (8[arg1,2] + 12))
    "movq   %%rsp, %%rbx\n\t"
    "lea    0xFFFFFFFFFFFFFFD8(%%rsp, %6), %%rsp\n\t"
    "subq   %5, %%rsp\n\t"

    // set up stack
    "movq   %1, 0x18(%%rsp)\n\t"  // s
    "movq   %2, 0x20(%%rsp)\n\t"  // format
    "lea    0x28(%%rsp), %%rdi\n\t"
    "movq   %%rdi, (%%rsp)\n\t"  // memcpy dest
    "movq   %5, 0x8(%%rsp)\n\t"  // memcpy src
    "movq   %5, 0x10(%%rsp)\n\t"
    "subq   %6, 0x10(%%rsp)\n\t"  // memcpy len
    "movq   0x10(%%rsp), %%r8\n\t"
    "movq   0x8(%%rsp), %%rdx\n\t"
    "movq   (%%rsp),  %%rcx\n\t"
    "call   " QUOTE(__MINGW_USYMBOL(memcpy)) "\n\t"
    "addq   $24, %%rsp\n\t"

    // call sscanf
    "movq   0x18(%%rsp), %%r9\n\t"
    "movq   0x10(%%rsp), %%r8\n\t"
    "movq   0x8(%%rsp), %%rdx\n\t"
    "movq   (%%rsp),  %%rcx\n\t"
    "call   " QUOTE(__MINGW_USYMBOL(sscanf)) "\n\t"

    // restore stack
    "movq   %%rbx, %%rsp\n\t"

    : "=a"(ret), "=c"(s), "=d"(format)
    : "1"(s), "2"(format), "S"(arg),
      "a"(&ret)
    : "rbx", "rdi");

  return ret;
}
\$\endgroup\$
  • \$\begingroup\$ I knew when I posted this that finding someone prepared to respond here was going to be a stretch. It's a highly obscure area that sensible people avoid. After running into reason 6, I have abandoned the inline asm approach altogether and have written a pure asm approach. But I could still use an informed source confirming that this code should be replaced. \$\endgroup\$ – David Wohlferd Jul 5 '16 at 22:58
3
\$\begingroup\$

As predicted, selling this change is turning out to be a challenge on my own, but that's life. I dislike leaving questions open, so I'm going to close my own question by quoting some specs that confirm my concerns:

  1. There isn't enough room reserved on the stack - True per spec:

    It contains at least four entries, but always enough space to hold all the parameters needed by any function that may be called. Note that space is always allocated for the register parameters, even if the parameters themselves are never homed to the stack; a callee is guaranteed that space has been allocated for all its parameters.

  2. Windows 64bit stacks are supposed to be 16 bit aligned - True per spec:

    The stack pointer must be aligned to 16 bytes, except for leaf functions, in any region of code that isn’t part of an epilog or prolog.

  3. rsp has a wild value - True per spec

    All memory beyond the current address of RSP is considered volatile: The OS, or a debugger, may overwrite this memory during a user debug session, or an interrupt handler. Thus, RSP must always be set before attempting to read or write values to a stack frame.

  4. Actual values are passed in registers - True per spec:

    The first four integer arguments are passed in registers.

    and

    The callee has the responsibility of dumping the register parameters into their shadow space if needed.

  5. The code only works because the distance between ptr and &ret just happens to be big enough - Debugging shows this to be true. The number of bytes copied varies between debug and retail builds, but increasing the number of parameters being passed has no effect. Once you get past a certain number of params (~20), the stack corrupts and the app crashes.

  6. Soes not include the appropriate .seh_* directives - Well, it surely doesn't include any of them. And it's modifying the stack size and calling functions, all of which normally require these directives. I can't say for sure that bad things will happen without them, but at a minimum I'm guessing it's bad practice.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.