Skip to content

[clang] Reject character devices in #embed for now #135370

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 15, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,9 @@ Bug Fixes in This Version

#if 1 ? 1 : 999999999999999999999
#endif
- ``#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. See (#GH126629).

Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
2 changes: 2 additions & 0 deletions clang/include/clang/Basic/DiagnosticLexKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -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">;
Comment on lines +461 to +462
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def ext_pp_extra_tokens_at_eol : ExtWarn<
"extra tokens at end of #%0 directive">, InGroup<ExtraTokens>;
Expand Down
6 changes: 6 additions & 0 deletions clang/include/clang/Basic/FileEntry.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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;
};
Expand All @@ -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(); }

Expand Down
2 changes: 2 additions & 0 deletions clang/lib/Basic/FileManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
6 changes: 6 additions & 0 deletions clang/lib/Lex/PPDirectives.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
16 changes: 16 additions & 0 deletions clang/test/Preprocessor/embed-reject-device-files-lin.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// RUN: %clang_cc1 -std=c23 %s -fsyntax-only -verify
// REQUIRES: system-linux


int urandom[] = {
#embed "/dev/urandom" //expected-error {{device files are not yet supported by '#embed' directive}}
};
int random[] = {
#embed "/dev/random" //expected-error {{device files are not yet supported by '#embed' directive}}
};
int zero[] = {
#embed "/dev/zero" //expected-error {{device files are not yet supported by '#embed' directive}}
};
int null[] = {
#embed "/dev/null" //expected-error {{device files are not yet supported by '#embed' directive}}
};
7 changes: 7 additions & 0 deletions clang/test/Preprocessor/embed-reject-device-files-win.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// RUN: %clang_cc1 -std=c23 %s -fsyntax-only -verify
// REQUIRES: system-windows


int null[] = {
#embed "NUL" limit(1) //expected-error {{device files are not yet supported by '#embed' directive}}
};
Loading