[llvm-objcopy] Fix crash for binary input files with non-ascii names

[jhenderson]

Summary

The code was using the standard isalnum function which doesn't handle values outside the non-ascii range. Switching to using llvm::isAlnum instead ensures we don't crash.

[MaskRay]

C11 says

The header <ctype.h> declares several functions useful for classifying and mapping characters.198) In all cases the argument is an int, the value of which shall be representable as an unsigned char or shall equal the value of the macro EOF. If the argument has any other value, the behavior is undefined.

The crash problem you described may be similar to https://drewdevault.com/2020/09/25/A-story-of-two-libcs.html
With certain values (negative ones?) glibc may crash.

llvm/tools/llvm-objcopy/ELF/Object.cpp

1293–1295

may need an unsigned char cast.

[jhenderson]

In D97663#2595340, @MaskRay wrote:

C11 says

The header <ctype.h> declares several functions useful for classifying and mapping characters.198) In all cases the argument is an int, the value of which shall be representable as an unsigned char or shall equal the value of the macro EOF. If the argument has any other value, the behavior is undefined.

The crash problem you described may be similar to https://drewdevault.com/2020/09/25/A-story-of-two-libcs.html
With certain values (negative ones?) glibc may crash.

Right, the the problem with the current code before the patch - by using a non-ascii character, some of the bytes of a multi-byte character invariably end up in the undefined range that causes problems with isalnum. It's basically the same as other issues I've been involved with in the past. Using llvm::isAlnum fixes the issue, since it doesn't have the same undefined behaviour.

llvm/tools/llvm-objcopy/ELF/Object.cpp

1293–1295

Why? The signature for isAlnum is bool isAlnum(char C).

[jhenderson]

llvm/tools/llvm-objcopy/ELF/Object.cpp

1292–1293

FYI - I've fixed this linter issue locally, just haven't bothered uploading a new diff to resolve it.

[MaskRay]

In D97663#2602894, @jhenderson wrote:

In D97663#2595340, @MaskRay wrote:

C11 says

The header <ctype.h> declares several functions useful for classifying and mapping characters.198) In all cases the argument is an int, the value of which shall be representable as an unsigned char or shall equal the value of the macro EOF. If the argument has any other value, the behavior is undefined.

The crash problem you described may be similar to https://drewdevault.com/2020/09/25/A-story-of-two-libcs.html
With certain values (negative ones?) glibc may crash.

Right, the the problem with the current code before the patch - by using a non-ascii character, some of the bytes of a multi-byte character invariably end up in the undefined range that causes problems with isalnum. It's basically the same as other issues I've been involved with in the past. Using llvm::isAlnum fixes the issue, since it doesn't have the same undefined behaviour.

OK, I did not check that llvm::isAlnum accepts char instead of int.

Still worth clarifying that this is fixing UB, which may be a crash on some implementations (glibc? Though I cannot reproduce it).

[jhenderson]

In D97663#2604828, @MaskRay wrote:

Still worth clarifying that this is fixing UB, which may be a crash on some implementations (glibc? Though I cannot reproduce it).

Okay, I'll update the commit message. For reference, this crashed on Windows using VS2017 for me.

llvm/test/tools/llvm-objcopy/ELF/binary-input.test

 
 ## The alignment can be changed by --set-section-alignment.
 # RUN: llvm-objcopy -I binary -O elf64-x86-64 --set-section-alignment .data=8 %t.x-txt %t2.o
 # RUN: llvm-readobj --sections %t2.o | FileCheck --check-prefix=ALIGN %s
 
 # ALIGN:      Name: .data
 # ALIGN:      AddressAlignment:
 # ALIGN-SAME:                   8{{$}}
+
+## Show that a filename with non-ASCII characters can be handled appropriately.
+## The exact encoding of the non-ASCII character will determine what characters
+## are used, so don't check for them specifically.
+# RUN: cp %t.x-txt %t€.x-txt
+# RUN: llvm-objcopy -I binary -O elf64-x86-64 %t€.x-txt %t3.o
+# RUN: llvm-readobj --sections --symbols %t3.o | FileCheck %s

llvm/tools/llvm-objcopy/ELF/Object.cpp

auto Data = ArrayRef<uint8_t>(
       reinterpret_cast<const uint8_t *>(MemBuf->getBufferStart()),
       MemBuf->getBufferSize());
   auto &DataSection = Obj->addSection<Section>(Data);
   DataSection.Name = ".data";
   DataSection.Type = ELF::SHT_PROGBITS;
   DataSection.Size = Data.size();
   DataSection.Flags = ELF::SHF_ALLOC | ELF::SHF_WRITE;
 
   std::string SanitizedFilename = MemBuf->getBufferIdentifier().str();
-  std::replace_if(std::begin(SanitizedFilename), std::end(SanitizedFilename),
-                  [](char C) { return !isalnum(C); }, '_');
+  std::replace_if(
+      std::begin(SanitizedFilename), std::end(SanitizedFilename),
+      [](char C) { return !isAlnum(C); }, '_');
   Twine Prefix = Twine("_binary_") + SanitizedFilename;
 
   SymTab->addSymbol(Prefix + "_start", STB_GLOBAL, STT_NOTYPE, &DataSection,
                     /*Value=*/0, NewSymbolVisibility, 0, 0);
   SymTab->addSymbol(Prefix + "_end", STB_GLOBAL, STT_NOTYPE, &DataSection,
                     /*Value=*/DataSection.Size, NewSymbolVisibility, 0, 0);
   SymTab->addSymbol(Prefix + "_size", STB_GLOBAL, STT_NOTYPE, nullptr,
                     /*Value=*/DataSection.Size, NewSymbolVisibility, SHN_ABS,