Parse icon_index from shortcuts.

Parse the existing icon_index of the
shortcut icon to preserve when the shortcut
is being reset.

Bug: 1116017, 1148962
Change-Id: Ia6528161bf4302e7abb5c5223c238ddb31856791
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2536033Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Commit-Queue: Bettina Dea <bdea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827454}

chrome/chrome_cleaner/mojom/parser_interface.mojom

@@ -27,7 +27,8 @@ interface Parser {
   ParseShortcut(handle<platform> lkn_file_handle)
    => (LnkParsingResult parsing_result, WString? target_path,
        WString? command_line_arguments,
-       WString? icon_location);
+       WString? icon_location,
+       int32 icon_index);
 
   // JSON parser:
   // Interface copied from services/data_decoder/public/mojom/json_parser.mojom,

chrome/chrome_cleaner/parsers/broker/lnk_parser_sandbox_setup_unittest.cc

@@ -78,7 +78,8 @@ TEST_F(LnkParserSandboxSetupTest, ParseCorrectShortcutSandboxedTest) {
 
   base::win::ShortcutProperties shortcut_properties;
   shortcut_properties.set_target(not_lnk_file_path_);
-  shortcut_properties.set_icon(not_lnk_file_path_, /*icon_index=*/0);
+  const int32_t icon_index = 0;
+  shortcut_properties.set_icon(not_lnk_file_path_, icon_index);
   const std::wstring lnk_arguments = L"argument1 -f -t -a -o";
   shortcut_properties.set_arguments(lnk_arguments);
 
@@ -97,7 +98,8 @@ TEST_F(LnkParserSandboxSetupTest, ParseCorrectShortcutSandboxedTest) {
 
   ASSERT_EQ(test_result_code_, mojom::LnkParsingResult::SUCCESS);
   EXPECT_TRUE(CheckParsedShortcut(test_parsed_shortcut_, not_lnk_file_path_,
-                                  lnk_arguments, not_lnk_file_path_));
+                                  lnk_arguments, not_lnk_file_path_,
+                                  icon_index));
 }
 
 TEST_F(LnkParserSandboxSetupTest, ParseIncorrectShortcutSandboxedTest) {
@@ -119,7 +121,8 @@ TEST_F(LnkParserSandboxSetupTest, ParseIncorrectShortcutSandboxedTest) {
 
   ASSERT_NE(test_result_code_, mojom::LnkParsingResult::SUCCESS);
   EXPECT_TRUE(CheckParsedShortcut(test_parsed_shortcut_, base::FilePath(L""),
-                                  L"", base::FilePath(L"")));
+                                  L"", base::FilePath(L""),
+                                  /*icon_index=*/-1));
 }
 
 }  // namespace chrome_cleaner

chrome/chrome_cleaner/parsers/shortcut_parser/broker/sandboxed_shortcut_parser.cc

@@ -110,7 +110,8 @@ void SandboxedShortcutParser::OnShortcutsParsingDone(
     mojom::LnkParsingResult parsing_result,
     const base::Optional<std::wstring>& optional_file_path,
     const base::Optional<std::wstring>& optional_command_line_arguments,
-    const base::Optional<std::wstring>& optional_icon_location) {
+    const base::Optional<std::wstring>& optional_icon_location,
+    int32_t icon_index) {
   ShortcutInformation parsed_shortcut;
   parsed_shortcut.lnk_path = lnk_path;
   if (parsing_result == mojom::LnkParsingResult::SUCCESS) {
@@ -121,9 +122,10 @@ void SandboxedShortcutParser::OnShortcutsParsingDone(
       parsed_shortcut.command_line_arguments =
           optional_command_line_arguments.value();
     }
-
-    if (optional_icon_location.has_value())
+    if (optional_icon_location.has_value()) {
       parsed_shortcut.icon_location = optional_icon_location.value();
+      parsed_shortcut.icon_index = icon_index;
+    }
 
     const std::wstring kChromeLnkName = L"Google Chrome.lnk";
     if (chrome_exe_locations.Contains(

chrome/chrome_cleaner/parsers/shortcut_parser/broker/sandboxed_shortcut_parser.h

@@ -45,7 +45,8 @@ class SandboxedShortcutParser : public ShortcutParserAPI {
       mojom::LnkParsingResult parsing_result,
       const base::Optional<std::wstring>& optional_file_path,
       const base::Optional<std::wstring>& optional_command_line_arguments,
-      const base::Optional<std::wstring>& optional_icon_location);
+      const base::Optional<std::wstring>& optional_icon_location,
+      int32_t icon_index);
 
   base::Lock lock_;
   MojoTaskRunner* mojo_task_runner_;

chrome/chrome_cleaner/parsers/shortcut_parser/broker/sandboxed_shortcut_parser_unittest.cc

@@ -257,7 +257,7 @@ TEST_F(SandboxedShortcutParserTest, ParseShortcutsWithSpecifiedIconsOnly) {
   base::win::ShortcutProperties reported_shortcut_properties;
   reported_shortcut_properties.set_target(not_lnk_file_path_);
   reported_shortcut_properties.set_icon(kSomeChromeIconLocation,
-                                        /*icon_index=*/0);
+                                        /*icon_index=*/1);
 
   base::win::ScopedHandle reported_shortcut_handle =
       CreateAndOpenShortcutInTempDir("reported shortcut.lnk",
@@ -284,6 +284,7 @@ TEST_F(SandboxedShortcutParserTest, ParseShortcutsWithSpecifiedIconsOnly) {
   ASSERT_EQ(found_shortcuts.size(), 1u);
   EXPECT_TRUE(PathEqual(base::FilePath(found_shortcuts[0].icon_location),
                         kSomeChromeIconLocation));
+  ASSERT_EQ(found_shortcuts[0].icon_index, 1);
 }
 
 }  // namespace chrome_cleaner

chrome/chrome_cleaner/parsers/shortcut_parser/broker/shortcut_parser_api.h

@@ -26,6 +26,7 @@ struct ShortcutInformation {
   std::wstring target_path;
   std::wstring command_line_arguments;
   std::wstring icon_location;
+  int32_t icon_index;
 };
 
 typedef base::OnceCallback<void(std::vector<ShortcutInformation>)>

chrome/chrome_cleaner/parsers/shortcut_parser/sandboxed_lnk_parser_test_util.cc

@@ -38,11 +38,13 @@ base::win::ScopedHandle CreateAndOpenShortcutInTempDir(
 bool CheckParsedShortcut(const ParsedLnkFile& parsed_shortcut,
                          base::FilePath expected_target_path,
                          std::wstring expected_arguments,
-                         base::FilePath expected_icon_location) {
+                         base::FilePath expected_icon_location,
+                         const int32_t expected_icon_index) {
   base::FilePath parsed_file_path(parsed_shortcut.target_path);
   return PathEqual(parsed_file_path, expected_target_path) &&
          (parsed_shortcut.command_line_arguments == expected_arguments) &&
-         PathEqual(parsed_file_path, expected_icon_location);
+         PathEqual(parsed_file_path, expected_icon_location) &&
+         (parsed_shortcut.icon_index == expected_icon_index);
 }
 
 void OnLnkParseDone(
@@ -52,7 +54,8 @@ void OnLnkParseDone(
     mojom::LnkParsingResult result_code,
     const base::Optional<std::wstring>& optional_file_path,
     const base::Optional<std::wstring>& optional_command_line_arguments,
-    const base::Optional<std::wstring>& optional_icon_location) {
+    const base::Optional<std::wstring>& optional_icon_location,
+    int32_t icon_index) {
   *out_result_code = result_code;
   if (optional_file_path.has_value())
     out_parsed_shortcut->target_path = optional_file_path.value();
@@ -62,8 +65,10 @@ void OnLnkParseDone(
         optional_command_line_arguments.value();
   }
 
-  if (optional_icon_location.has_value())
+  if (optional_icon_location.has_value()) {
     out_parsed_shortcut->icon_location = optional_icon_location.value();
+    out_parsed_shortcut->icon_index = icon_index;
+  }
 
   std::move(callback).Run();
 }

chrome/chrome_cleaner/parsers/shortcut_parser/sandboxed_lnk_parser_test_util.h

@@ -26,7 +26,8 @@ base::win::ScopedHandle CreateAndOpenShortcutInTempDir(
 bool CheckParsedShortcut(const ParsedLnkFile& parsed_shortcut,
                          base::FilePath expected_target_path,
                          std::wstring expected_arguments,
-                         base::FilePath expected_icon_location);
+                         base::FilePath expected_icon_location,
+                         const int32_t icon_index);
 void OnLnkParseDone(
     ParsedLnkFile* out_parsed_shortcut,
     mojom::LnkParsingResult* out_result_code,
@@ -34,7 +35,8 @@ void OnLnkParseDone(
     mojom::LnkParsingResult result_code,
     const base::Optional<std::wstring>& optional_file_path,
     const base::Optional<std::wstring>& optional_command_line_arguments,
-    const base::Optional<std::wstring>& optional_icon_location);
+    const base::Optional<std::wstring>& optional_icon_location,
+    int32_t optional_icon_index);
 
 }  // namespace chrome_cleaner
 

chrome/chrome_cleaner/parsers/shortcut_parser/target/lnk_parser.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/chrome_cleaner/parsers/shortcut_parser/target/lnk_parser.h"
 
+#include <string.h>
 #include <windows.h>
 
 #include <memory>
@@ -339,16 +340,21 @@ mojom::LnkParsingResult internal::ParseLnkBytes(
   }
 
   // Retrieve the icon location.
-  if (has_icon_location &&
-      !ReadUtf16StringStructure(file_buffer, &current_byte,
-                                &parsed_shortcut->icon_location)) {
-    LOG(ERROR) << "Error reading icon location";
-    return mojom::LnkParsingResult::BAD_FORMAT;
+  if (has_icon_location) {
+    if (!ReadUtf16StringStructure(file_buffer, &current_byte,
+                                  &parsed_shortcut->icon_location)) {
+      LOG(ERROR) << "Error reading icon location";
+      return mojom::LnkParsingResult::BAD_FORMAT;
+    } else {
+      parsed_shortcut->icon_index = lnk_file_header->icon_index;
+    }
   }
 
   return mojom::LnkParsingResult::SUCCESS;
 }
 
+ParsedLnkFile::ParsedLnkFile() {}
+
 // Please note that the documentation used to write this parser was obtained
 // from the following link:
 // https://msdn.microsoft.com/en-us/library/dd871305.aspx

chrome/chrome_cleaner/parsers/shortcut_parser/target/lnk_parser.h

@@ -68,9 +68,11 @@ mojom::LnkParsingResult ParseLnkBytes(std::vector<BYTE> file_buffer,
 }  // namespace internal
 
 struct ParsedLnkFile {
+  ParsedLnkFile();
   std::wstring target_path;
   std::wstring command_line_arguments;
   std::wstring icon_location;
+  int32_t icon_index = -1;
 };
 
 mojom::LnkParsingResult ParseLnk(base::win::ScopedHandle file_handle,

chrome/chrome_cleaner/parsers/shortcut_parser/target/lnk_parser_unittest.cc

@@ -86,7 +86,8 @@ class LnkParserTest : public testing::Test {
   void CheckParsedShortcut(ParsedLnkFile* parsed_shortcut,
                            base::FilePath target_path,
                            std::wstring arguments,
-                           base::FilePath icon_location) {
+                           base::FilePath icon_location,
+                           const int32_t& icon_index) {
     base::FilePath parsed_file_path(parsed_shortcut->target_path);
     ASSERT_TRUE(PathEqual(parsed_file_path, target_path));
 
@@ -94,6 +95,7 @@ class LnkParserTest : public testing::Test {
 
     base::FilePath parsed_icon_location(parsed_shortcut->icon_location);
     ASSERT_TRUE(PathEqual(parsed_icon_location, icon_location));
+    ASSERT_EQ(parsed_shortcut->icon_index, icon_index);
   }
 
   bool CreateFileWithUTF16Name(base::FilePath* file_path) {
@@ -314,7 +316,7 @@ TEST_F(LnkParserTest, ParseLnkWithoutArgumentsTest) {
   ASSERT_EQ(ParseLnk(std::move(lnk_handle), &parsed_shortcut),
             mojom::LnkParsingResult::SUCCESS);
   CheckParsedShortcut(&parsed_shortcut, target_file_path_, L"",
-                      base::FilePath(L""));
+                      base::FilePath(L""), properties.icon_index);
 }
 
 TEST_F(LnkParserTest, ParseLnkWithArgumentsTest) {
@@ -330,7 +332,7 @@ TEST_F(LnkParserTest, ParseLnkWithArgumentsTest) {
   ASSERT_EQ(ParseLnk(std::move(lnk_handle), &parsed_shortcut),
             mojom::LnkParsingResult::SUCCESS);
   CheckParsedShortcut(&parsed_shortcut, target_file_path_, kArguments,
-                      base::FilePath(L""));
+                      base::FilePath(L""), properties.icon_index);
 }
 
 TEST_F(LnkParserTest, ParseLnkWithExtraStringStructures) {
@@ -350,7 +352,7 @@ TEST_F(LnkParserTest, ParseLnkWithExtraStringStructures) {
   ASSERT_EQ(ParseLnk(std::move(lnk_handle), &parsed_shortcut),
             mojom::LnkParsingResult::SUCCESS);
   CheckParsedShortcut(&parsed_shortcut, target_file_path_, kArguments,
-                      base::FilePath(L""));
+                      base::FilePath(L""), properties.icon_index);
 }
 
 TEST_F(LnkParserTest, UTF16FileNameParseTest) {
@@ -364,7 +366,8 @@ TEST_F(LnkParserTest, UTF16FileNameParseTest) {
   ParsedLnkFile parsed_shortcut;
   ASSERT_EQ(ParseLnk(std::move(lnk_handle), &parsed_shortcut),
             mojom::LnkParsingResult::SUCCESS);
-  CheckParsedShortcut(&parsed_shortcut, utf16_path, L"", base::FilePath(L""));
+  CheckParsedShortcut(&parsed_shortcut, utf16_path, L"", base::FilePath(L""),
+                      properties.icon_index);
 }
 
 TEST_F(LnkParserTest, InvalidHandleTest) {
@@ -413,7 +416,7 @@ TEST_F(LnkParserTest, ReasonablyLargeFileSizeShortcutTest) {
   ASSERT_EQ(ParseLnk(std::move(lnk_handle), &parsed_shortcut),
             mojom::LnkParsingResult::SUCCESS);
   CheckParsedShortcut(&parsed_shortcut, target_file_path_, L"",
-                      base::FilePath(L""));
+                      base::FilePath(L""), properties.icon_index);
 }
 
 TEST_F(LnkParserTest, TooLargeFileSizeShortcutTest) {
@@ -505,7 +508,7 @@ TEST_F(LnkParserTest, ArgumentsSizeCorruptedShortcutTest_Smaller) {
   ASSERT_EQ(ParseLnk(std::move(lnk_handle), &parsed_shortcut),
             mojom::LnkParsingResult::SUCCESS);
   CheckParsedShortcut(&parsed_shortcut, target_file_path_, L"foo",
-                      base::FilePath(L""));
+                      base::FilePath(L""), properties.icon_index);
 }
 
 TEST_F(LnkParserTest, LocalAndNetworkShortcutTest) {
@@ -521,7 +524,7 @@ TEST_F(LnkParserTest, LocalAndNetworkShortcutTest) {
   ASSERT_EQ(ParseLnk(std::move(local_and_network_lnk_handle), &parsed_shortcut),
             mojom::LnkParsingResult::SUCCESS);
   CheckParsedShortcut(&parsed_shortcut, target_file_path_, L"",
-                      base::FilePath(L""));
+                      base::FilePath(L""), properties.icon_index);
 }
 
 TEST_F(LnkParserTest, ParseIconLocationTest) {
@@ -530,9 +533,10 @@ TEST_F(LnkParserTest, ParseIconLocationTest) {
                                          base::File::Flags::FLAG_READ);
   ASSERT_TRUE(txt_file.IsValid());
 
+  int32_t icon_index = 0;
   base::win::ShortcutProperties properties;
   properties.set_target(txt_file_path);
-  properties.set_icon(txt_file_path, /*icon_index=*/0);
+  properties.set_icon(txt_file_path, icon_index);
 
   base::win::ScopedHandle txt_lnk_handle = CreateAndOpenShortcut(properties);
   ASSERT_TRUE(txt_lnk_handle.IsValid());
@@ -540,7 +544,8 @@ TEST_F(LnkParserTest, ParseIconLocationTest) {
   ParsedLnkFile parsed_shortcut;
   EXPECT_EQ(ParseLnk(std::move(txt_lnk_handle), &parsed_shortcut),
             mojom::LnkParsingResult::SUCCESS);
-  CheckParsedShortcut(&parsed_shortcut, txt_file_path, L"", txt_file_path);
+  CheckParsedShortcut(&parsed_shortcut, txt_file_path, L"", txt_file_path,
+                      icon_index);
 }
 
 }  // namespace internal

chrome/chrome_cleaner/parsers/target/parser_impl.cc

@@ -44,7 +44,8 @@ void ParserImpl::ParseShortcut(mojo::PlatformHandle lnk_file_handle,
     std::move(callback).Run(mojom::LnkParsingResult::INVALID_HANDLE,
                             base::make_optional<std::wstring>(),
                             base::make_optional<std::wstring>(),
-                            base::make_optional<std::wstring>());
+                            base::make_optional<std::wstring>(),
+                            /*icon_index=*/-1);
     return;
   }
 
@@ -56,14 +57,15 @@ void ParserImpl::ParseShortcut(mojo::PlatformHandle lnk_file_handle,
     LOG(ERROR) << "Error parsing the shortcut";
     std::move(callback).Run(result, base::make_optional<std::wstring>(),
                             base::make_optional<std::wstring>(),
-                            base::make_optional<std::wstring>());
+                            base::make_optional<std::wstring>(),
+                            /*icon_index=*/-1);
     return;
   }
-
   std::move(callback).Run(
       result, base::make_optional<std::wstring>(parsed_shortcut.target_path),
       base::make_optional<std::wstring>(parsed_shortcut.command_line_arguments),
-      base::make_optional<std::wstring>(parsed_shortcut.icon_location));
+      base::make_optional<std::wstring>(parsed_shortcut.icon_location),
+      parsed_shortcut.icon_index);
 }
 
 }  // namespace chrome_cleaner

chrome/chrome_cleaner/parsers/target/parser_impl_unittest.cc

@@ -126,9 +126,10 @@ TEST_F(ParserImplTest, ParseJsonError) {
 }
 
 TEST_F(ParserImplTest, ParseCorrectShortcutTest) {
+  const int32_t icon_index = 1;
   base::win::ShortcutProperties shortcut_properties;
   shortcut_properties.set_target(not_lnk_file_path_);
-  shortcut_properties.set_icon(not_lnk_file_path_, /*icon_index=*/0);
+  shortcut_properties.set_icon(not_lnk_file_path_, icon_index);
   const std::wstring lnk_arguments = L"argument1 -f -t -a -o";
   shortcut_properties.set_arguments(lnk_arguments);
 
@@ -145,7 +146,8 @@ TEST_F(ParserImplTest, ParseCorrectShortcutTest) {
 
   ASSERT_EQ(test_result_code_, mojom::LnkParsingResult::SUCCESS);
   EXPECT_TRUE(CheckParsedShortcut(test_parsed_shortcut_, not_lnk_file_path_,
-                                  lnk_arguments, not_lnk_file_path_));
+                                  lnk_arguments, not_lnk_file_path_,
+                                  icon_index));
 }
 
 TEST_F(ParserImplTest, ParseIncorrectShortcutTest) {
@@ -166,6 +168,7 @@ TEST_F(ParserImplTest, ParseIncorrectShortcutTest) {
 
   ASSERT_NE(test_result_code_, mojom::LnkParsingResult::SUCCESS);
   EXPECT_TRUE(CheckParsedShortcut(test_parsed_shortcut_, base::FilePath(L""),
-                                  L"", base::FilePath(L"")));
+                                  L"", base::FilePath(L""),
+                                  /*icon_index=*/-1));
 }
 }  // namespace chrome_cleaner