[clang] Reject character devices in #embed for now

[Fznamznon]

See #126629 . Right not they are not supported properly and support requires modifying several layers of LLVM. For now simply reject them while proper support is being developed to avoid potential security problems.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Mariya Podchishchaeva (Fznamznon)

Changes

See #126629 . Right not they are not supported properly and support requires modifying several layers of LLVM. For now simply reject them while proper support is being developed to avoid potential security problems.

---

Full diff: https://github.com/llvm/llvm-project/pull/135370.diff

4 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticLexKinds.td (+2)
• (modified) clang/include/clang/Basic/FileEntry.h (+6)
• (modified) clang/lib/Basic/FileManager.cpp (+2)
• (modified) clang/lib/Lex/PPDirectives.cpp (+6)

diff --git a/clang/include/clang/Basic/DiagnosticLexKinds.td b/clang/include/clang/Basic/DiagnosticLexKinds.td
index 912b8bd46e194..f29edfa835d42 100644
--- a/clang/include/clang/Basic/DiagnosticLexKinds.td
+++ b/clang/include/clang/Basic/DiagnosticLexKinds.td
@@ -458,6 +458,8 @@ def warn_compat_pp_embed_directive : Warning<
   InGroup<CPre23Compat>, DefaultIgnore;
 def err_pp_embed_dup_params : Error<
   "cannot specify parameter '%0' twice in the same '#embed' directive">;
+def err_pp_embed_device_file : Error<
+  "device files are not yet supported by '#embed' directive">;
 
 def ext_pp_extra_tokens_at_eol : ExtWarn<
   "extra tokens at end of #%0 directive">, InGroup<ExtraTokens>;
diff --git a/clang/include/clang/Basic/FileEntry.h b/clang/include/clang/Basic/FileEntry.h
index da5ba90974293..c973ba38bdf7e 100644
--- a/clang/include/clang/Basic/FileEntry.h
+++ b/clang/include/clang/Basic/FileEntry.h
@@ -82,6 +82,7 @@ class FileEntryRef {
   inline const llvm::sys::fs::UniqueID &getUniqueID() const;
   inline time_t getModificationTime() const;
   inline bool isNamedPipe() const;
+  inline bool isDeviceFile() const;
   inline void closeFile() const;
 
   /// Check if the underlying FileEntry is the same, intentially ignoring
@@ -316,6 +317,7 @@ class FileEntry {
   llvm::sys::fs::UniqueID UniqueID;
   unsigned UID = 0; // A unique (small) ID for the file.
   bool IsNamedPipe = false;
+  bool IsDeviceFile = false;
 
   /// The open file, if it is owned by the \p FileEntry.
   mutable std::unique_ptr<llvm::vfs::File> File;
@@ -340,6 +342,7 @@ class FileEntry {
   /// Check whether the file is a named pipe (and thus can't be opened by
   /// the native FileManager methods).
   bool isNamedPipe() const { return IsNamedPipe; }
+  bool isDeviceFile() const { return IsDeviceFile; }
 
   void closeFile() const;
 };
@@ -357,6 +360,9 @@ time_t FileEntryRef::getModificationTime() const {
 }
 
 bool FileEntryRef::isNamedPipe() const { return getFileEntry().isNamedPipe(); }
+bool FileEntryRef::isDeviceFile() const {
+  return getFileEntry().isDeviceFile();
+}
 
 void FileEntryRef::closeFile() const { getFileEntry().closeFile(); }
 
diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index ec84aad72e6be..86fe352df0461 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -330,6 +330,8 @@ llvm::Expected<FileEntryRef> FileManager::getFileRef(StringRef Filename,
   UFE->UID = NextFileUID++;
   UFE->UniqueID = Status.getUniqueID();
   UFE->IsNamedPipe = Status.getType() == llvm::sys::fs::file_type::fifo_file;
+  UFE->IsDeviceFile =
+      Status.getType() == llvm::sys::fs::file_type::character_file;
   UFE->File = std::move(F);
 
   if (UFE->File) {
diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp
index d97a6e8d64f9c..318b8fdad2e04 100644
--- a/clang/lib/Lex/PPDirectives.cpp
+++ b/clang/lib/Lex/PPDirectives.cpp
@@ -4010,6 +4010,12 @@ void Preprocessor::HandleEmbedDirective(SourceLocation HashLoc, Token &EmbedTok,
     Diag(FilenameTok, diag::err_pp_file_not_found) << Filename;
     return;
   }
+
+  if (MaybeFileRef->isDeviceFile()) {
+    Diag(FilenameTok, diag::err_pp_embed_device_file) << Filename;
+    return;
+  }
+
   std::optional<llvm::MemoryBufferRef> MaybeFile =
       getSourceManager().getMemoryBufferForFileOrNone(*MaybeFileRef);
   if (!MaybeFile) {

[Fznamznon]

I'm not sure what to say in the release note. Sorry folks, we never really supported these but now we reject to be safe?

[AaronBallman]

I think we should add some test coverage for both Windows and Linux. I think on Linux we can use /dev/urandom and on Windows we could use something like COM1 on Windows. WDYT?

Also, these changes should come with a release note.

[AaronBallman]

I'm not sure what to say in the release note. Sorry folks, we never really supported these but now we reject to be safe?

I would say something like "the '#embed' directive now diagnoses use of a non-character file (device file) such as /dev/urandom as an error. This restriction may be relaxed in the future." or something along those lines, with a link to the GitHub issue.

[Fznamznon]

Oh no, I didn't pick up the test

[Fznamznon]

on Windows we could use something like COM1 on Windows.

How would that look like?

[AaronBallman]

on Windows we could use something like COM1 on Windows.

How would that look like?

#embed "COM1" limit(1)

But when I try that on my Windows machine, I get a file not found error. Perhaps try #embed "NUL" limit(1)?

[AaronBallman]

LGTM assuming precommit CI comes back green. Thank you!

[cor3ntin]

Thank you so much for working on that!

[cor3ntin]

Suggested change

	def err_pp_embed_device_file : Error<
	"device files are not yet supported by '#embed' directive">;
	def err_pp_embed_device_file : Error<
	"device files are not supported by '#embed' directive">;

Can we remove yet? It's unclear that it's a direction that we will ever want to go in, and I don't want to set the wrong expectations

[AaronBallman]

It's the intent of the feature to work with device files (these examples come from the original paper), so I think "yet" sets the correct expectations.