Truncate file name longer than MAX_PATH(260) on D2D

ui/base/dragdrop/os_exchange_data_provider_win.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/base/dragdrop/os_exchange_data_provider_win.h"
 
+#include <algorithm>
+#include <vector>
+
 #include "base/file_path.h"
 #include "base/i18n/file_util_icu.h"
 #include "base/logging.h"
 #include "base/memory/scoped_handle.h"
 #include "base/pickle.h"
 #include "base/stl_util.h"
 #include "base/utf_string_conversions.h"
 #include "base/win/scoped_hglobal.h"
 #include "googleurl/src/gurl.h"
 #include "grit/ui_strings.h"
@@ skipping 100 matching lines @@
 }
 
 FormatEtcEnumerator::~FormatEtcEnumerator() {
   STLDeleteContainerPointers(contents_.begin(), contents_.end());
 }
 
 STDMETHODIMP FormatEtcEnumerator::Next(
     ULONG count, FORMATETC* elements_array, ULONG* elements_fetched) {
   // MSDN says |elements_fetched| is allowed to be NULL if count is 1.
   if (!elements_fetched)
-    DCHECK(count == 1);
+    DCHECK_EQ(count, 1ul);
 
   // This method copies count elements into |elements_array|.
   ULONG index = 0;
   while (cursor_ < contents_.size() && index < count) {
     CloneFormatEtc(contents_[cursor_], &elements_array[index]);
     ++cursor_;
     ++index;
   }
   // The out param is for how many we actually copied.
   if (elements_fetched)
@@ skipping 258 matching lines @@
 bool OSExchangeDataProviderWin::GetPickledData(CLIPFORMAT format,
                                                Pickle* data) const {
   DCHECK(data);
   FORMATETC format_etc =
       { format, NULL, DVASPECT_CONTENT, -1, TYMED_HGLOBAL };
   bool success = false;
   STGMEDIUM medium;
   if (SUCCEEDED(source_object_->GetData(&format_etc, &medium))) {
     if (medium.tymed & TYMED_HGLOBAL) {
       base::win::ScopedHGlobal<char> c_data(medium.hGlobal);
-      DCHECK(c_data.Size() > 0);
+      DCHECK_GT(c_data.Size(), static_cast<size_t>(0));

[sky, 2011/09/08 17:25:46]

0u

[yosin_UTC9, 2011/09/09 04:25:41]

Done.

       // Need to subtract 1 as SetPickledData adds an extra byte to the end.
       *data = Pickle(c_data.get(), static_cast<int>(c_data.Size() - 1));
       success = true;
     }
     ReleaseStgMedium(&medium);
   }
   return success;
 }
 
 bool OSExchangeDataProviderWin::GetFileContents(
@@ skipping 465 matching lines @@
   static const size_t max_length = MAX_PATH - arraysize(extension);
   if (validated->size() > max_length)
     validated->erase(max_length);
   *validated += extension;
 }
 
 static STGMEDIUM* GetStorageForFileName(const FilePath& path) {
   const size_t kDropSize = sizeof(DROPFILES);
   const size_t kTotalBytes =
       kDropSize + (path.value().length() + 2) * sizeof(wchar_t);
   HANDLE hdata = GlobalAlloc(GMEM_MOVEABLE, kTotalBytes);

[sky, 2011/09/08 17:25:46]

Shouldn't we check the return value?

[yosin_UTC9, 2011/09/09 04:25:41]

Yes, we should check. Although, I would like to do this in another CR. Checking
is easy but we also need to change upstream to handling failure case. It isn't
simple change and many places (at least four places in this file.)

I would like to focus this CR to fix buffer overflow.

 
   base::win::ScopedHGlobal<DROPFILES> locked_mem(hdata);
   DROPFILES* drop_files = locked_mem.get();
   drop_files->pFiles = sizeof(DROPFILES);
   drop_files->fWide = TRUE;
   wchar_t* data = reinterpret_cast<wchar_t*>(
       reinterpret_cast<BYTE*>(drop_files) + kDropSize);
   const size_t copy_size = (path.value().length() + 1) * sizeof(wchar_t);
   memcpy(data, path.value().c_str(), copy_size);
   data[path.value().length() + 1] = L'\0';  // Double NULL
 
   STGMEDIUM* storage = new STGMEDIUM;
   storage->tymed = TYMED_HGLOBAL;
-  storage->hGlobal = drop_files;
+  storage->hGlobal = hdata;
   storage->pUnkForRelease = NULL;
   return storage;
 }
 
 static STGMEDIUM* GetStorageForFileDescriptor(
     const FilePath& path) {
-  string16 valid_file_name = path.value();
-  DCHECK(!valid_file_name.empty() && valid_file_name.size() + 1 <= MAX_PATH);
-  HANDLE handle = GlobalAlloc(GPTR, sizeof(FILEGROUPDESCRIPTOR));
-  FILEGROUPDESCRIPTOR* descriptor =
-      reinterpret_cast<FILEGROUPDESCRIPTOR*>(GlobalLock(handle));
+  string16 file_name = path.value();
+  DCHECK(!file_name.empty());
+  HANDLE hdata = GlobalAlloc(GPTR, sizeof(FILEGROUPDESCRIPTOR));

[sky, 2011/09/08 17:25:46]

Same thing about checking return value here.

[yosin_UTC9, 2011/09/09 04:25:41]

I'll do another CR.

+  base::win::ScopedHGlobal<FILEGROUPDESCRIPTOR> locked_mem(hdata);
 
+  FILEGROUPDESCRIPTOR* descriptor = locked_mem.get();
   descriptor->cItems = 1;
-  wcscpy_s(descriptor->fgd[0].cFileName,
-           valid_file_name.size() + 1,
-           valid_file_name.c_str());
   descriptor->fgd[0].dwFlags = FD_LINKUI;
-
-  GlobalUnlock(handle);
+  int file_name_len = file_name.size();
+  if (file_name_len + 1 < MAX_PATH) {
+    wcscpy_s(descriptor->fgd[0].cFileName,
+             MAX_PATH,
+             file_name.c_str());
+  } else {
+    string16 extension = path.Extension();

[sky, 2011/09/08 17:25:46]

Shouldn't we make sure we maintain the path component? For example, lets say
path is something like:

c:/temp/foo.an-absurdly-long-extension-that-is-almost-max_path

Shouldn't we make sure we honor the c:/temp part?

[yosin_UTC9, 2011/09/09 04:25:41]

Callers should take care of it. Actually, this function receives only file name
component at this time, even if filename parameter in Content-Disposition.

+    int extension_len = extension.size();
+    int max_name_len = MAX_PATH - extension_len - 1;
+    if (max_name_len >= 1) {
+      // We take at least one character from name.
+      int orig_name_len = file_name_len - extension_len;
+      int name_len = std::min(orig_name_len, max_name_len);
+      memcpy(descriptor->fgd[0].cFileName,
+             file_name.c_str(),
+             sizeof(wchar_t) * name_len);
+      wcscpy_s(descriptor->fgd[0].cFileName + name_len,
+               MAX_PATH - extension_len,
+               extension.c_str());
+    } else {
+      // We take MAX_PATH-1 characters from file_name.
+      wcsncpy_s(descriptor->fgd[0].cFileName,
+                MAX_PATH,
+                file_name.c_str(),
+                MAX_PATH - 1);
+    }
+  }
 
   STGMEDIUM* storage = new STGMEDIUM;
-  storage->hGlobal = handle;
+  storage->hGlobal = hdata;
   storage->tymed = TYMED_HGLOBAL;
   storage->pUnkForRelease = NULL;
   return storage;
 }
 
-
 ///////////////////////////////////////////////////////////////////////////////
 // OSExchangeData, public:
 
 // static
 OSExchangeData::Provider* OSExchangeData::CreateProvider() {
   return new OSExchangeDataProviderWin();
 }
 
 // static
 OSExchangeData::CustomFormat OSExchangeData::RegisterCustomFormat(
     const std::string& type) {
   return RegisterClipboardFormat(ASCIIToUTF16(type).c_str());
 }
 
 }  // namespace ui