Instrumentation support for PIE & shared libs on x86_64 Linux

[vleonen]

This PR enables instrumentation support for PIE executables, shared libraries, and initial support for statically linked executables on x86_64 Linux systems.
We are using relative jumps in trampoline functions to avoid usage of absolute addresses calculated during build time on the linking stage for initialization/finalization functions and instrumentation calls. These absolute addresses are not valid for randomized addresses for PIE & shared libraries.
Also for correct operation for instrumented shared libraries, we added a set of syscalls and functions for searching a path to the current binary in /proc/self/map_files directory based on the current virtual address and symlinks in this directory. In case/proc/self/map_files directory access is restricted (e.g. in container environment), we added -instrumentation-binpath option to pass information about expected path to an instrumented binary during runtime.

Co-authored-by: Vladislav Khmelevsky [email protected]
Co-authored-by: Elvina Yakubova [email protected]

Fixes #137

Vasily Leonenko,
Advanced Software Technology Lab, Huawei

[rafaelauler]

Thanks for working on this! I've left some suggestions below, let me know what do you think.

[rafaelauler]

Let's follow LLVM coding standards here (see https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly ) and use DirPath[] instead.

Similarly, target_path should be TargetPath, and so on. The only place in the instrumentation code that we are not following these guidelines is in the name of syscalls, such as __open, but that's a special symbol. So __readlink and __getdents (introduced in another patch in this PR). are good and shouldn't be changed.

[ElvinaYakubova]

Thanks for the suggestions :)

[rafaelauler]

This reads a little bit like C code when we initialize all variables at the start of the function. Because this is a c++ file that follows LLVM style, it makes more sense to push some of these definitions closer to where they are used. For example, LenS declaration can be moved to line 610, LenE declaration can be moved to line 621, etc.

I do understand that this file has very low-level code that looks more like systems programming (where C is very popular) and less like compiler code, but even LLVM has some low-level libraries that still follows strictly C++ styles, example: https://github.com/facebookincubator/BOLT/blob/main/llvm/lib/Support/Unix/Program.inc

[rafaelauler]

Code that does parsing gets complicated and hard to read because it usually needs to keep several pointers to the parsing buffer. To avoid this from distracting us from the main purpose of this function, what do you think about refactoring this in a "parseAddressRange()" helper function or a lambda? We can write it in such a way that it returns StartAddress and EndAddress as integers directly, and then add a comment that it is parsing <hex1>-<hex2>. I always find it helpful to add comments in parsing functions to make it clear what is the format that the code is trying to read

[ElvinaYakubova]

Good idea, will do. Thank you

[rafaelauler]

Remove this for (;;) here, I don't think we need to re-execute this loop body, right?

[ElvinaYakubova]

Here is an example at the end of the page https://man7.org/linux/man-pages/man2/getdents.2.html and it seems like we need this loop (getdents can read to the buf only BUF_SIZE elements at once), otherwise we might lose some entries.

[rafaelauler]

Sorry, I missed the line

     if (nread == 0)
      break;

with the exit condition. So it reads until nread is zero. Then perhaps this outer loop could be written as:

while (nread = __getdents(FDdir, (struct dirent *)buf, BUF_SIZE) ) {
}

To make the exit condition more explicit

[rafaelauler]

Put these as const integers inside getBinaryPath():

const uint32_t BufSize = 1024;
const uint32_t NameMax = 256;
const uint32_t AddrSize = 32;

[rafaelauler]

The goto here shouldn't be necessary once we remove the outer loop. It will be just one loop checking all entries, it will be something like:

for ( entries start... end boundaries...) {
  uint64_t StartAddress, EndAddress;
  if (!parseAddressRange(StartAddress, EndAddress))
    continue;
  if (CurAddr < StartAddress || CurAddr > EndAddress)
    continue;
  const *C = strCopy(FindBuf, DirPath, NameMaX]);
  C = strCopy(C, d->d_name, NameMax - (C - FindBuf)]);
  *C = '\0';
  uint32_t Ret = __readlink(FindBuf, TargetPath, sizeof(TargetPath));
  assert(Ret != -1 && Ret != BufSize, "readlink error");
  TargetPath[Ret] = '\0';
  return TargetPath;
}
return nulptr;
 

[rafaelauler]

res -> Res
str -> Str

[rafaelauler]

Here it would be nice to refactor a bit:

  for (const ELF64LE::Phdr &Phdr : PHs) {
    if (Phdr.p_type == ELF::PT_INTERP) {
      BC->hasInterpHeader = true;
      continue;
    }
    if (Phdr.p_type != ELF::PT_LOAD)
      continue;
    BC->FirstAllocAddress = std::min(BC->FirstAllocAddress,
                                     static_cast<uint64_t>(Phdr.p_vaddr));
    NextAvailableAddress = std::max(NextAvailableAddress,
                                    Phdr.p_vaddr + Phdr.p_memsz);
    NextAvailableOffset = std::max(NextAvailableOffset,
                                   Phdr.p_offset + Phdr.p_filesz);
    BC->SegmentMapInfo[Phdr.p_vaddr] = SegmentInfo{Phdr.p_vaddr,
                                                   Phdr.p_memsz,
                                                   Phdr.p_offset,
                                                   Phdr.p_filesz,
                                                   Phdr.p_align};
  }

or a switch-case.

[rafaelauler]

Capitalize:

HasInterpHeader

[rafaelauler]

tranpoline -> trampoline

[rafaelauler]

I think I finished reviewing the most important parts of this PR. Let me know what you think about the comments and thanks a lot for working on this, that was a great job.

[rafaelauler]

Can you add a comment here with the assembly code being generated by this function? In the same way as lines 3247-3260 for createInstrumentedTailCallHandler(), so it's easier to read. It is a bit distracting to read and visualize the assembly code that it is trying to emit because the function needs to emplace_back each instr.

[rafaelauler]

The "Instr" here confused me a bit because I was associating with "Instr" being "Instruction". Perhaps we can rename to IndCallCounterFuncPtr? (since it is basically just updating a counter)

[rafaelauler]

It took me a while to understand what was happening here because we are not creating two functions but the real intent is to create two basic blocks of the same function that is coordinating entry to the code that performs instrumentation of indirect calls.

Can we rename this variable to IndCallHandlerExitBB, and add a comment explaning? Also, the methods "createInstrumentedIndCallHandler" and "createInstrumentedIndCallTrampoline" could be renamed to "createInstrumentedIndCalllHandlerExitBB" and "createInstrumentedIndCallHandlerEntryBB", respectively, so it's extra clear what we are doing here.

Regarding the comment explaining, I was thinking in something like:

// Here IndCallHandlerExitBB has the instructions to finish handling traffic
// to an indirect call. We pass it to createInstrumentedIndCallHandlerEntryBB(), which
// will decide between jumping to code in the runtime library (to account traffic)
// if the pointer is set, and then finish by jumping to the exit bb, which will return
// normal program execution.

[rafaelauler]

Same

[vleonen]

@rafaelauler Thank you for reviewing it. I just have pushed a version updated according to your comments and rebased on top of current main branch. Please have a look at it.

[rafaelauler]

Thanks a lot folks!

[vleonen]

@rafaelauler Thank you!
I just found that readme file in root and in bolt folder become duplicated some time ago. And after my commit they become different (this PR patch 1437d30 modifies bolt/README.md). Please give me know if I need to prepare one more PR to sync them or do you plan to remove bolt/README.md to avoid further duplication and uncertainly?

[rafaelauler]

Oh, good catch! Let me fix this.