Skip to content
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

[libc++][test] try to directly create socket file in /tmp when filepath is too long #77058

Merged
merged 8 commits into from
Jan 9, 2024

Conversation

yingcong-wu
Copy link
Contributor

@yingcong-wu yingcong-wu commented Jan 5, 2024

If TMP is set to a folder which path is too long, the current libcxx test helper function create_socket() will fail because of the test temp folder test_root's path is too long to be used in socket creation.
In such case, this patch will try to create the socket file directly in /tmp folder.

This patch also add an assertion for bind().

@yingcong-wu yingcong-wu changed the title [libcxx][test] try to directly create socket file in /tmp when filepath is too long [libc++][test] try to directly create socket file in /tmp when filepath is too long Jan 5, 2024
Copy link

github-actions bot commented Jan 5, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@yingcong-wu yingcong-wu marked this pull request as ready for review January 5, 2024 10:50
@yingcong-wu yingcong-wu requested a review from a team as a code owner January 5, 2024 10:50
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jan 5, 2024
@llvmbot
Copy link

llvmbot commented Jan 5, 2024

@llvm/pr-subscribers-libcxx

Author: Wu Yingcong (yingcong-wu)

Changes

If TMP is set to a folder which path is too long, the current libcxx test helper function create_socket() will fail because of the test temp folder test_root's path is too long to be used in socket creation.
In such case, this patch will try to create the socket file directly in /tmp folder.


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

1 Files Affected:

  • (modified) libcxx/test/support/filesystem_test_helper.h (+21-8)
diff --git a/libcxx/test/support/filesystem_test_helper.h b/libcxx/test/support/filesystem_test_helper.h
index a049efe03d844e..e472eced8217a1 100644
--- a/libcxx/test/support/filesystem_test_helper.h
+++ b/libcxx/test/support/filesystem_test_helper.h
@@ -18,6 +18,7 @@
 #include <chrono>
 #include <cstdint>
 #include <cstdio> // for printf
+#include <ctime>
 #include <string>
 #include <system_error>
 #include <type_traits>
@@ -320,15 +321,27 @@ struct scoped_test_env
   // allow tests to call this unguarded.
 #if !defined(__FreeBSD__) && !defined(__APPLE__) && !defined(_WIN32)
     std::string create_socket(std::string file) {
-        file = sanitize_path(std::move(file));
+      std::string socket_file = sanitize_path(file);
 
-        ::sockaddr_un address;
-        address.sun_family = AF_UNIX;
-        assert(file.size() <= sizeof(address.sun_path));
-        ::strncpy(address.sun_path, file.c_str(), sizeof(address.sun_path));
-        int fd = ::socket(AF_UNIX, SOCK_STREAM, 0);
-        ::bind(fd, reinterpret_cast<::sockaddr*>(&address), sizeof(address));
-        return file;
+      ::sockaddr_un address;
+      address.sun_family = AF_UNIX;
+
+      // If file.size() is too big, try to create a file directly inside
+      // /tmp to make sure file path is short enough.
+      if (socket_file.size() <= sizeof(address.sun_path) && utils::exists("/tmp")) {
+        fs::path const tmp = "/tmp";
+        std::size_t i      = std::hash<std::string>()(std::to_string(std::time(nullptr)));
+        fs::path p         = tmp / ("libcxx-socket-" + file + "-" + std::to_string(i));
+        while (utils::exists(p.string())) {
+          p = tmp / ("libcxx-socket-" + file + "-" + std::to_string(++i));
+        }
+        socket_file = p.string();
+      }
+      assert(socket_file.size() <= sizeof(address.sun_path));
+      ::strncpy(address.sun_path, socket_file.c_str(), sizeof(address.sun_path));
+      int fd = ::socket(AF_UNIX, SOCK_STREAM, 0);
+      assert(::bind(fd, reinterpret_cast<::sockaddr*>(&address), sizeof(address)) == 0);
+      return socket_file;
     }
 #endif
 

Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

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

Does this problem happen in practice? If so on what platform did this occur?

libcxx/test/support/filesystem_test_helper.h Outdated Show resolved Hide resolved
@mordante mordante self-assigned this Jan 7, 2024
@yingcong-wu
Copy link
Contributor Author

Does this problem happen in practice? If so on what platform did this occur?

My problem happened on Linux when I set a TEMP folder with some long path.
I think the test should handle long TEMP, and the rest of tests are fine. Only the socket creation fails because there are a length limit in the socket file path.

Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM!

// If file.size() is too big, try to create a file directly inside
// /tmp to make sure file path is short enough.
// Android platform warns about tmpnam, since the problem does not appear
// on Android, let's not apply it for Android.
Copy link
Member

Choose a reason for hiding this comment

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

Not too happy about this, but since we assert afterwards it will fail the test. If that happens we can investigate.

@yingcong-wu
Copy link
Contributor Author

Great, thank you. I don't have commit access, could you please help land this patch for me?

@mordante mordante merged commit a43e0f9 into llvm:main Jan 9, 2024
43 checks passed
@mordante
Copy link
Member

mordante commented Jan 9, 2024

Done.

@yingcong-wu
Copy link
Contributor Author

Thank you very much.

mordante pushed a commit that referenced this pull request Jan 12, 2024
A logical mistake is made in #77058, we should try to find a new file
path for socket creation when the path's length generated is bigger than
the socket length limit.
@yingcong-wu yingcong-wu deleted the yc/libcxx-socket-path branch January 22, 2024 06:11
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
…th is too long (llvm#77058)

If TMP is set to a folder which path is too long, the current libcxx
test helper function `create_socket()` will fail because of the test
temp folder `test_root`'s path is too long to be used in socket
creation.
In such case, this patch will try to create the socket file directly in
`/tmp` folder.

This patch also add an assertion for `bind()`.
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
…7867)

A logical mistake is made in llvm#77058, we should try to find a new file
path for socket creation when the path's length generated is bigger than
the socket length limit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants