[LogicalView] - Crash when loading a library

[aurelien35]

In the file https://github.com/llvm/llvm-project/blob/main/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp

There is the "LVReaderHandler::handleArchive" method (around line 73) which is used to load debug informations from libraries.

The first statement create a local "Error" variable:

Error LVReaderHandler::handleArchive(LVReaders &Readers, StringRef Filename,
                                     Archive &Arch) {
  
  // HERE
  Error Err = Error::success();


  for (const Archive::Child &Child : Arch.children(Err)) {
    Expected<MemoryBufferRef> BuffOrErr = Child.getMemoryBufferRef();
    if (Error Err = BuffOrErr.takeError())
      return createStringError(errorToErrorCode(std::move(Err)), "%s",
                               Filename.str().c_str());
    Expected<StringRef> NameOrErr = Child.getName();
    if (Error Err = NameOrErr.takeError())
      return createStringError(errorToErrorCode(std::move(Err)), "%s",
                               Filename.str().c_str());
    std::string Name = (Filename + "(" + NameOrErr.get() + ")").str();
    if (Error Err = handleBuffer(Readers, Name, BuffOrErr.get()))
      return createStringError(errorToErrorCode(std::move(Err)), "%s",
                               Filename.str().c_str());
  }

  return Error::success();
}

But this error is never checked or returned.
So the variable is always destroyed (without being moved) when it goes out of scope, which leads to an exception since the error checking is now mandatory before its deletion, even if tis a success.

I've tried a simple fix which seems to to the job, but I don't know if it's the best way to fix this issue:

Error LVReaderHandler::handleArchive(LVReaders &Readers, StringRef Filename,
                                     Archive &Arch) {
  Error Err = Error::success();

  for (const Archive::Child &Child : Arch.children(Err)) {
    Expected<MemoryBufferRef> BuffOrErr = Child.getMemoryBufferRef();
    if (Error Err = BuffOrErr.takeError())
      return createStringError(errorToErrorCode(std::move(Err)), "%s",
                               Filename.str().c_str());
    Expected<StringRef> NameOrErr = Child.getName();
    if (Error Err = NameOrErr.takeError())
      return createStringError(errorToErrorCode(std::move(Err)), "%s",
                               Filename.str().c_str());
    std::string Name = (Filename + "(" + NameOrErr.get() + ")").str();
    if (Error Err = handleBuffer(Readers, Name, BuffOrErr.get()))
      return createStringError(errorToErrorCode(std::move(Err)), "%s",
                               Filename.str().c_str());
  }

  // FIX HERE : the variable is always checked
  if (Err)
  {
     return Error::success();
  }

  return Err;
}

[llvmbot]

@llvm/issue-subscribers-debuginfo

Author: None (aurelien35)

In the file [https://github.com/llvm/llvm-project/blob/main/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp](https://github.com/llvm/llvm-project/blob/main/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp)

There is the "LVReaderHandler::handleArchive" method (around line 73) which is used to load debug informations from libraries.

The first statement create a local "Error" variable:

Error LVReaderHandler::handleArchive(LVReaders &Readers, StringRef Filename,
                                     Archive &Arch) {
  
  // HERE
  Error Err = Error::success();


  for (const Archive::Child &Child : Arch.children(Err)) {
    Expected<MemoryBufferRef> BuffOrErr = Child.getMemoryBufferRef();
    if (Error Err = BuffOrErr.takeError())
      return createStringError(errorToErrorCode(std::move(Err)), "%s",
                               Filename.str().c_str());
    Expected<StringRef> NameOrErr = Child.getName();
    if (Error Err = NameOrErr.takeError())
      return createStringError(errorToErrorCode(std::move(Err)), "%s",
                               Filename.str().c_str());
    std::string Name = (Filename + "(" + NameOrErr.get() + ")").str();
    if (Error Err = handleBuffer(Readers, Name, BuffOrErr.get()))
      return createStringError(errorToErrorCode(std::move(Err)), "%s",
                               Filename.str().c_str());
  }

  return Error::success();
}

But this error is never checked or returned.
So the variable is always destroyed (without being moved) when it goes out of scope, which leads to an exception since the error checking is now mandatory before its deletion, even if tis a success.

I've tried a simple fix which seems to to the job, but I don't know if it's the best way to fix this issue:

Error LVReaderHandler::handleArchive(LVReaders &Readers, StringRef Filename,
                                     Archive &Arch) {
  Error Err = Error::success();

  for (const Archive::Child &Child : Arch.children(Err)) {
    Expected<MemoryBufferRef> BuffOrErr = Child.getMemoryBufferRef();
    if (Error Err = BuffOrErr.takeError())
      return createStringError(errorToErrorCode(std::move(Err)), "%s",
                               Filename.str().c_str());
    Expected<StringRef> NameOrErr = Child.getName();
    if (Error Err = NameOrErr.takeError())
      return createStringError(errorToErrorCode(std::move(Err)), "%s",
                               Filename.str().c_str());
    std::string Name = (Filename + "(" + NameOrErr.get() + ")").str();
    if (Error Err = handleBuffer(Readers, Name, BuffOrErr.get()))
      return createStringError(errorToErrorCode(std::move(Err)), "%s",
                               Filename.str().c_str());
  }

  // FIX HERE : the variable is always checked
  if (Err)
  {
     return Error::success();
  }

  return Err;
}

[pogo59]

@CarlosAlbertoEnciso
Probably the thing to do is just delete the local variable. It is shadowed by declarations in the if statements and never used.

[aurelien35]

It was my first thought, but this variable is used in the first for loop :

for (const Archive::Child &Child : Arch.children(Err)) {

Maybe it should be checked on each loop iteration and returned on the first error?

[CarlosAlbertoEnciso]

The code to load a library is quite similar to the one used by llvm-cgdata and llvm-dwarfdump

A suggested fix would be:

  Error Err = Error::success();
  for (const Archive::Child &Child : Arch.children(Err)) {
    ...
  }

  if (Err)
    return createStringError(errorToErrorCode(std::move(Err)), "%s",
                               Filename.str().c_str());
  return Error::success();

[aurelien35]

I've made a pull request here: #118951