Prevent crash when failed to create output dir.

PiperOrigin-RevId: 712709379

connections/implementation/internal_payload_factory.cc

@@ -76,7 +76,7 @@ class BytesInternalPayload : public InternalPayload {
   }
 
   ExceptionOr<size_t> SkipToOffset(size_t offset) override {
-    NEARBY_LOGS(WARNING) << "Bytes payload does not support offsets";
+    LOG(WARNING) << "Bytes payload does not support offsets";
     return {Exception::kIo};
   }
 
@@ -115,8 +115,8 @@ class OutgoingStreamInternalPayload : public InternalPayload {
     ByteArray scoped_bytes_read = std::move(bytes_read.result());
 
     if (scoped_bytes_read.Empty()) {
-      NEARBY_LOGS(INFO) << "No more data for outgoing payload " << this
-                        << ", closing InputStream.";
+      LOG(INFO) << "No more data for outgoing payload " << this
+                << ", closing InputStream.";
 
       input_stream->Close();
       return {};
@@ -142,9 +142,8 @@ class OutgoingStreamInternalPayload : public InternalPayload {
     if (!real_offset.ok()) {
       return real_offset;
     }
-    NEARBY_LOGS(WARNING) << "Skip offset: " << real_offset.GetResult()
-                         << ", expected offset: " << offset << " for payload "
-                         << this;
+    LOG(WARNING) << "Skip offset: " << real_offset.GetResult()
+                 << ", expected offset: " << offset << " for payload " << this;
     return {Exception::kIo};
   }
 
@@ -172,8 +171,8 @@ class IncomingStreamInternalPayload : public InternalPayload {
 
   Exception AttachNextChunk(const ByteArray& chunk) override {
     if (chunk.Empty()) {
-      NEARBY_LOGS(INFO) << "Received null last chunk for incoming payload "
-                        << this << ", closing OutputStream.";
+      LOG(INFO) << "Received null last chunk for incoming payload " << this
+                << ", closing OutputStream.";
       Close();
       return {Exception::kSuccess};
     }
@@ -182,8 +181,7 @@ class IncomingStreamInternalPayload : public InternalPayload {
   }
 
   ExceptionOr<size_t> SkipToOffset(size_t offset) override {
-    NEARBY_LOGS(WARNING) << "Cannot skip offset for an incoming Payload "
-                         << this;
+    LOG(WARNING) << "Cannot skip offset for an incoming Payload " << this;
     return {Exception::kIo};
   }
 
@@ -234,7 +232,7 @@ class OutgoingFileInternalPayload : public InternalPayload {
   }
 
   ExceptionOr<size_t> SkipToOffset(size_t offset) override {
-    NEARBY_LOGS(INFO) << "SkipToOffset " << offset;
+    LOG(INFO) << "SkipToOffset " << offset;
     InputFile* file = payload_.AsFile();
     if (!file) {
       return {Exception::kIo};
@@ -249,9 +247,9 @@ class OutgoingFileInternalPayload : public InternalPayload {
     if (!real_offset.ok()) {
       return real_offset;
     }
-    NEARBY_LOGS(WARNING) << "Skip offset: " << real_offset.GetResult()
-                         << ", expected offset: " << offset
-                         << " for file payload " << this;
+    LOG(WARNING) << "Skip offset: " << real_offset.GetResult()
+                 << ", expected offset: " << offset << " for file payload "
+                 << this;
     return {Exception::kIo};
   }
 
@@ -294,8 +292,7 @@ class IncomingFileInternalPayload : public InternalPayload {
   }
 
   ExceptionOr<size_t> SkipToOffset(size_t offset) override {
-    NEARBY_LOGS(WARNING) << "Cannot skip offset for an incoming file Payload "
-                         << this;
+    LOG(WARNING) << "Cannot skip offset for an incoming file Payload " << this;
     return {Exception::kIo};
   }
 
@@ -399,8 +396,8 @@ ErrorOr<std::unique_ptr<InternalPayload>> CreateIncomingInternalPayload(
         } else {
           // This is an error condition, we don't have any way to generate a
           // file name for the output file.
-          NEARBY_LOGS(ERROR) << "File name not found in incoming file Payload, "
-                                "and the Id wasn't found.";
+          LOG(ERROR) << "File name not found in incoming file Payload, "
+                        "and the Id wasn't found.";
           return {Error(OperationResultCode::IO_FILE_OPENING_ERROR)};
         }
       }
@@ -413,14 +410,24 @@ ErrorOr<std::unique_ptr<InternalPayload>> CreateIncomingInternalPayload(
       // there will be no input file to open.
       // On Chrome the file path should be empty, so use the payload id.
       if (ImplementationPlatform::GetCurrentOS() == OSName::kChromeOS) {
+        OutputFile output_file(payload_id);
+        if (!output_file.IsValid()) {
+          LOG(ERROR) << "Output file payload ID is not valid: " << payload_id;
+          return {Error(OperationResultCode::IO_FILE_OPENING_ERROR)};
+        }
         return {std::make_unique<IncomingFileInternalPayload>(
             Payload(payload_id, InputFile(payload_id, total_size)),
-            OutputFile(payload_id), total_size)};
+            std::move(output_file), total_size)};
       } else {
+        OutputFile output_file(file_path);
+        if (!output_file.IsValid()) {
+          LOG(ERROR) << "Output file payload path is not valid: " << file_path;
+          return {Error(OperationResultCode::IO_FILE_OPENING_ERROR)};
+        }
         return {std::make_unique<IncomingFileInternalPayload>(
             Payload(payload_id, parent_folder, file_name,
                     InputFile(file_path, total_size)),
-            OutputFile(file_path), total_size)};
+            std::move(output_file), total_size)};
       }
     }
     default:

connections/implementation/internal_payload_factory_test.cc

@@ -132,7 +132,7 @@ TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromStreamMessage) {
 
 TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromFileMessage) {
   PayloadTransferFrame frame;
-  std::string path = "C:\\Downloads";
+  std::string path = "/tmp/Downloads";
   frame.set_packet_type(PayloadTransferFrame::DATA);
   auto& header = *frame.mutable_payload_header();
   header.set_type(PayloadTransferFrame::PayloadHeader::FILE);
@@ -153,7 +153,7 @@ TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromFileMessage) {
 TEST(InternalPayloadFactoryTest,
      InternalPayloadFromFileMessageWithoutIdReturnsNullptr) {
   PayloadTransferFrame frame;
-  std::string path = "C:\\Downloads";
+  std::string path = "/tmp/Downloads";
   frame.set_packet_type(PayloadTransferFrame::DATA);
   auto& header = *frame.mutable_payload_header();
   header.set_type(PayloadTransferFrame::PayloadHeader::FILE);
@@ -166,7 +166,7 @@ TEST(InternalPayloadFactoryTest,
 TEST(InternalPayloadFactoryTest,
      CanCreateInternalPayloadFromFileMessageWithFileNameNotSet) {
   PayloadTransferFrame frame;
-  std::string path = "C:\\Downloads";
+  std::string path = "/tmp/Downloads";
   frame.set_packet_type(PayloadTransferFrame::DATA);
   auto& header = *frame.mutable_payload_header();
   header.set_type(PayloadTransferFrame::PayloadHeader::FILE);
@@ -180,10 +180,11 @@ TEST(InternalPayloadFactoryTest,
   Payload payload = internal_payload->ReleasePayload();
   EXPECT_EQ(payload.GetFileName(), "12345");
 }
+
 TEST(InternalPayloadFactoryTest,
      CanCreateInternalPayloadFromFileMessageWithFileNameSet) {
   PayloadTransferFrame frame;
-  std::string path = "C:\\Downloads";
+  std::string path = "/tmp/Downloads";
   frame.set_packet_type(PayloadTransferFrame::DATA);
   auto& header = *frame.mutable_payload_header();
   header.set_type(PayloadTransferFrame::PayloadHeader::FILE);
@@ -200,6 +201,23 @@ TEST(InternalPayloadFactoryTest,
   EXPECT_EQ(payload.GetFileName(), "test.file.name");
 }
 
+TEST(InternalPayloadFactoryTest,
+     CreateInternalPayloadFailsIfFileCannotBeCreated) {
+  PayloadTransferFrame frame;
+  // /dev/null is a special file, no sub directories can be created
+  std::string path = "/dev/null";
+  frame.set_packet_type(PayloadTransferFrame::DATA);
+  auto& header = *frame.mutable_payload_header();
+  header.set_type(PayloadTransferFrame::PayloadHeader::FILE);
+  header.set_id(12345);
+  header.set_total_size(512);
+  header.set_file_name("test.file.name");
+  header.set_parent_folder("Downloads2");
+  ErrorOr<std::unique_ptr<InternalPayload>> result =
+      CreateIncomingInternalPayload(frame, path);
+  ASSERT_TRUE(result.has_error());
+}
+
 void CreateFileWithContents(Payload::Id payload_id, const ByteArray& contents) {
   OutputFile file(payload_id);
   EXPECT_TRUE(file.Write(contents).Ok());

internal/platform/file.cc

@@ -61,6 +61,7 @@ OutputFile::~OutputFile() = default;
 OutputFile::OutputFile(OutputFile&&) noexcept = default;
 OutputFile& OutputFile::operator=(OutputFile&&) = default;
 
+bool OutputFile::IsValid() const { return impl_ != nullptr; }
 // Writes all data from ByteArray object to the underlying stream.
 // Returns Exception::kIo on error, Exception::kSuccess otherwise.
 Exception OutputFile::Write(const ByteArray& data) {

internal/platform/file.h

@@ -77,6 +77,8 @@ class OutputFile final {
   OutputFile(OutputFile&&) noexcept;
   OutputFile& operator=(OutputFile&&);
 
+  bool IsValid() const;
+
   // Writes all data from ByteArray object to the underlying stream.
   // Returns Exception::kIo on error, Exception::kSuccess otherwise.
   Exception Write(const ByteArray& data);

internal/platform/implementation/g3/BUILD

@@ -155,19 +155,19 @@ cc_library(
         ":comm",
         ":crypto",  # build_cleaner: keep
         ":types",
+        "//internal/base:files",
         "//internal/platform:base",
+        "//internal/platform:logging",
         "//internal/platform:test_util",
         "//internal/platform/implementation:comm",
         "//internal/platform/implementation:platform",
         "//internal/platform/implementation:types",
         "//internal/platform/implementation/shared:count_down_latch",
         "//internal/platform/implementation/shared:file",
         "@com_google_absl//absl/base:core_headers",
-        "@com_google_absl//absl/memory",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/strings",
-        "@com_google_absl//absl/time",
         "@com_google_nisaba//nisaba/port:thread_pool",
     ],
 )

internal/platform/implementation/g3/platform.cc

@@ -14,19 +14,18 @@
 
 #include "internal/platform/implementation/platform.h"
 
-#include <atomic>
 #include <cstddef>
 #include <cstdint>
+#include <filesystem>  // NOLINT
 #include <memory>
 #include <string>
 
 #include "absl/base/attributes.h"
-#include "absl/memory/memory.h"
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
 #include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
-#include "absl/time/time.h"
+#include "internal/base/files.h"
 #include "internal/platform/implementation/atomic_boolean.h"
 #include "internal/platform/implementation/atomic_reference.h"
 #include "internal/platform/implementation/ble.h"
@@ -51,6 +50,7 @@
 #include "internal/platform/implementation/wifi_direct.h"
 #include "internal/platform/implementation/wifi_hotspot.h"
 #include "internal/platform/implementation/wifi_lan.h"
+#include "internal/platform/logging.h"
 #include "internal/platform/os_name.h"
 #include "internal/platform/payload_id.h"
 #include "thread/thread.h"
@@ -86,7 +86,7 @@ namespace api {
 
 std::string ImplementationPlatform::GetCustomSavePath(
     const std::string& parent_folder, const std::string& file_name) {
-  return absl::StrCat(parent_folder, file_name);
+  return absl::StrCat(parent_folder, "/", file_name);
 }
 
 std::string ImplementationPlatform::GetDownloadPath(
@@ -162,6 +162,15 @@ std::unique_ptr<OutputFile> ImplementationPlatform::CreateOutputFile(
 
 std::unique_ptr<OutputFile> ImplementationPlatform::CreateOutputFile(
     const std::string& file_path) {
+  std::filesystem::path path = std::filesystem::u8path(file_path);
+  std::filesystem::path folder_path = path.parent_path();
+  // Verifies that a path is a valid directory.
+  if (!sharing::DirectoryExists(folder_path)) {
+    if (!sharing::CreateDirectories(folder_path)) {
+      LOG(ERROR) << "Failed to create directory: " << folder_path.string();
+      return nullptr;
+    }
+  }
   return shared::IOFile::CreateOutputFile(file_path);
 }