Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2088006:
Give the extension unpacker process a junction/symlink free path to the unpack directory. (Closed)
|
Created:
10 years, 7 months ago by Sam Kerner (Chrome) Modified:
9 years, 7 months ago CC:
chromium-reviews, Alexander Potapenko, ben+cc_chromium.org, John Grabowski, Erik does not do reviews, Paweł Hajdan Jr., Timur Iskhodzhanov, Aaron Boodman, stuartmorgan, pam+watch_chromium.org, brettw-cc_chromium.org Visibility:
Public. |
DescriptionGive the extension unpacker process a junction/symlink free path to the unpack directory.
BUG=35198, 13044
TEST=FileUtilTest.NormalizeFilePathBasic,FileUtilTest. NormalizeFilePathReparsePoints,FileUtilTest.NormalizeFilePathSymlinks
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=49337
Patch Set 1 : Fix unit test on mac. #Patch Set 2 : Add comment with link to msdn article explaing the implementation od RealFilePath(). #
Total comments: 26
Patch Set 3 : Uses anon namespaces for static functions. #Patch Set 4 : Update unit test name. #Patch Set 5 : Update a variable name. #
Total comments: 4
Patch Set 6 : Add junction unittests. #
Total comments: 16
Patch Set 7 : Fixes for unit tests. #Patch Set 8 : FIx up unit tests. #
Total comments: 2
Patch Set 9 : Move path prefixing inoto unit test helper. #
Total comments: 3
Patch Set 10 : Rebase for commit. #
Messages
Total messages: 19 (0 generated)
Carlos, This change implements your recommendation on issue 13044, comment #22: > Comment 22 by cpu@chromium.org, Aug 18, 2009 > Yes the sandbox disallows redirection via junctions. > Basically the path the policy refers and the > resolved path differ so that is a security red flag. > > The best solution is to resolve the junction and > add that path to the policy instead. Sam
Let me pull in Ricardo, he is best to review the Windows piece. On 2010/05/14 22:29:27, Sam Kerner (Chrome) wrote: > Carlos, > This change implements your recommendation on issue 13044, comment #22: > > > Comment 22 by mailto:cpu@chromium.org, Aug 18, 2009 > > Yes the sandbox disallows redirection via junctions. > > Basically the path the policy refers and the > > resolved path differ so that is a security red flag. > > > > The best solution is to resolve the junction and > > add that path to the policy instead. > > Sam
. On 2010/05/17 17:52:38, cpu wrote: > Let me pull in Ricardo, he is best to review the Windows piece. > > On 2010/05/14 22:29:27, Sam Kerner (Chrome) wrote: > > Carlos, > > This change implements your recommendation on issue 13044, comment #22: > > > > > Comment 22 by mailto:cpu@chromium.org, Aug 18, 2009 > > > Yes the sandbox disallows redirection via junctions. > > > Basically the path the policy refers and the > > > resolved path differ so that is a security red flag. > > > > > > The best solution is to resolve the junction and > > > add that path to the policy instead. > > > > Sam
http://codereview.chromium.org/2088006/diff/8001/1013 File base/file_util_win.cc (right): http://codereview.chromium.org/2088006/diff/8001/1013#newcode912 base/file_util_win.cc:912: FilePath* drive_letter_path) { prefer unnamed namespaces to static. See http://winterdom.com/dev/cpp/nspaces This applies to the other cases in the other files of this CL
http://codereview.chromium.org/2088006/diff/8001/1013 File base/file_util_win.cc (right): http://codereview.chromium.org/2088006/diff/8001/1013#newcode912 base/file_util_win.cc:912: FilePath* drive_letter_path) { On 2010/05/17 17:58:15, cpu wrote: > prefer unnamed namespaces to static. See http://winterdom.com/dev/cpp/nspaces > > This applies to the other cases in the other files of this CL > Fixed here and in file_util_posix.cc .
http://codereview.chromium.org/2088006/diff/8001/1010 File base/file_util.h (right): http://codereview.chromium.org/2088006/diff/8001/1010#newcode282 base/file_util.h:282: // Set |real_path| to |path| with symbolic links and junctions expanded. nit: Sets http://codereview.chromium.org/2088006/diff/8001/1012 File base/file_util_unittest.cc (right): http://codereview.chromium.org/2088006/diff/8001/1012#newcode419 base/file_util_unittest.cc:419: // This can only be done on POSIX platforms. So no test for Windows? http://codereview.chromium.org/2088006/diff/8001/1013 File base/file_util_win.cc (right): http://codereview.chromium.org/2088006/diff/8001/1013#newcode916 base/file_util_win.cc:916: if (!::GetLogicalDriveStrings(kDriveMappingSize-1, drive_mapping)) { nit: spaces around '-' http://codereview.chromium.org/2088006/diff/8001/1013#newcode924 base/file_util_win.cc:924: wchar_t device_junction[MAX_PATH]; nit: device_name instead? http://codereview.chromium.org/2088006/diff/8001/1013#newcode953 base/file_util_win.cc:953: FILE_SHARE_READ, relax the sharing. http://codereview.chromium.org/2088006/diff/8001/1013#newcode956 base/file_util_win.cc:956: FILE_FLAG_BACKUP_SEMANTICS, why do you want this?. http://codereview.chromium.org/2088006/diff/8001/1013#newcode962 base/file_util_win.cc:962: // from a file handle. If we ever depricate XP, consider changing the typo deprecate http://codereview.chromium.org/2088006/diff/8001/1013#newcode980 base/file_util_win.cc:980: 1, // Just one page. No need to look at the data. nit: 1 byte http://codereview.chromium.org/2088006/diff/8001/1013#newcode997 base/file_util_win.cc:997: wchar_t mapped_file_path[MAX_PATH+1]; this may not be enough. http://codereview.chromium.org/2088006/diff/8001/1013#newcode1005 base/file_util_win.cc:1005: // to detect and resolve the junction. This whole comment is backwards... \Device\ is the "real" path, the others are links. http://codereview.chromium.org/2088006/diff/8001/1013#newcode1012 base/file_util_win.cc:1012: nit: remove this line
Ready for another look. http://codereview.chromium.org/2088006/diff/8001/1010 File base/file_util.h (right): http://codereview.chromium.org/2088006/diff/8001/1010#newcode282 base/file_util.h:282: // Set |real_path| to |path| with symbolic links and junctions expanded. On 2010/05/17 19:55:38, rvargas wrote: > nit: Sets Done. http://codereview.chromium.org/2088006/diff/8001/1012 File base/file_util_unittest.cc (right): http://codereview.chromium.org/2088006/diff/8001/1012#newcode419 base/file_util_unittest.cc:419: // This can only be done on POSIX platforms. On 2010/05/17 19:55:38, rvargas wrote: > So no test for Windows? FileUtilTest.RealFilePathBasic tests the tricky part of the windows code: * opening a file handle. * memory mapping the file. * changing the device path to a lettered drive path. It does not test with a path that would make the memory mapped object give a different path. To do that, the test would need to create a junction. I could not find any documented way to make a junction from C++. I tested this manually with the sysinternals tool junction.exe. There is a guy who reverse engineered the undocumented API required to make junctions, and it would be possible to use what he learned. But it is quite messy: http://www.codeproject.com/KB/winsdk/junctionpoints.aspx Given the fact that GetMappedFileName() has to do the wrong thing to make the test fail, messing with undocumented APIs to make it possible to do the test doesn't seem worth it. http://codereview.chromium.org/2088006/diff/8001/1013 File base/file_util_win.cc (right): http://codereview.chromium.org/2088006/diff/8001/1013#newcode916 base/file_util_win.cc:916: if (!::GetLogicalDriveStrings(kDriveMappingSize-1, drive_mapping)) { On 2010/05/17 19:55:38, rvargas wrote: > nit: spaces around '-' Done. http://codereview.chromium.org/2088006/diff/8001/1013#newcode924 base/file_util_win.cc:924: wchar_t device_junction[MAX_PATH]; On 2010/05/17 19:55:38, rvargas wrote: > nit: device_name instead? Done. http://codereview.chromium.org/2088006/diff/8001/1013#newcode953 base/file_util_win.cc:953: FILE_SHARE_READ, On 2010/05/17 19:55:38, rvargas wrote: > relax the sharing. Done. http://codereview.chromium.org/2088006/diff/8001/1013#newcode956 base/file_util_win.cc:956: FILE_FLAG_BACKUP_SEMANTICS, On 2010/05/17 19:55:38, rvargas wrote: > why do you want this?. It was needed when the code was used for a different approach to the problem. Changed to FILE_ATTRIBUTE_NORMAL. http://codereview.chromium.org/2088006/diff/8001/1013#newcode962 base/file_util_win.cc:962: // from a file handle. If we ever depricate XP, consider changing the On 2010/05/17 19:55:38, rvargas wrote: > typo deprecate Done. http://codereview.chromium.org/2088006/diff/8001/1013#newcode980 base/file_util_win.cc:980: 1, // Just one page. No need to look at the data. On 2010/05/17 19:55:38, rvargas wrote: > nit: 1 byte Done. http://codereview.chromium.org/2088006/diff/8001/1013#newcode997 base/file_util_win.cc:997: wchar_t mapped_file_path[MAX_PATH+1]; On 2010/05/17 19:55:38, rvargas wrote: > this may not be enough. I think your concern is that the <path> in "\Device\harddisk2\<path>" is limited to MAX_PATH wchars, but the whole thing is longer. Is there a better constant to use, or should I just add a constant to MAX_PATH? http://codereview.chromium.org/2088006/diff/8001/1013#newcode1005 base/file_util_win.cc:1005: // to detect and resolve the junction. On 2010/05/17 19:55:38, rvargas wrote: > This whole comment is backwards... \Device\ is the "real" path, the others are > links. That makes a lot more sense. http://codereview.chromium.org/2088006/diff/8001/1013#newcode1012 base/file_util_win.cc:1012: On 2010/05/17 19:55:38, rvargas wrote: > nit: remove this line Done.
http://codereview.chromium.org/2088006/diff/8001/1012 File base/file_util_unittest.cc (right): http://codereview.chromium.org/2088006/diff/8001/1012#newcode419 base/file_util_unittest.cc:419: // This can only be done on POSIX platforms. On 2010/05/17 22:53:05, Sam Kerner (Chrome) wrote: > On 2010/05/17 19:55:38, rvargas wrote: > > So no test for Windows? > > FileUtilTest.RealFilePathBasic tests the tricky part of the windows code: > > * opening a file handle. > * memory mapping the file. > * changing the device path to a lettered drive path. > > It does not test with a path that would make the memory mapped object give a > different path. To do that, the test would need to create a junction. > > I could not find any documented way to make a junction from C++. I tested > this manually with the sysinternals tool junction.exe. There is a guy who > reverse engineered the undocumented API required to make junctions, and it would > be possible to use what he learned. But it is quite messy: > http://www.codeproject.com/KB/winsdk/junctionpoints.aspx > > Given the fact that GetMappedFileName() has to do the wrong thing to make the > test fail, messing with undocumented APIs to make it possible to do the test > doesn't seem worth it. You could use CreateSymbolicLink and limit the test to Vista+ (make sure that you can delete it afterwards), or issue a FSCTL_SET_REPARSE_POINT ioctl (copy the code from src/sandbox/test/common/test_util.cc) http://codereview.chromium.org/2088006/diff/8001/1013 File base/file_util_win.cc (right): http://codereview.chromium.org/2088006/diff/8001/1013#newcode997 base/file_util_win.cc:997: wchar_t mapped_file_path[MAX_PATH+1]; On 2010/05/17 22:53:05, Sam Kerner (Chrome) wrote: > On 2010/05/17 19:55:38, rvargas wrote: > > this may not be enough. > > I think your concern is that the <path> in "\Device\harddisk2\<path>" is limited > to MAX_PATH wchars, but the whole thing is longer. Is there a better constant > to use, or should I just add a constant to MAX_PATH? <path> is not really limited to MAX_PATH either, but in general we fail in that case already; but yes, this code could introduce new failures due to the expansion of any part of the path (not just the drive). I guess adding a few bytes to the array (and clearly documenting the limitation) is good enough, but I would also verify the behavior of this code when the result exceeds the size of the provided buffer (and MAX_PATH). http://codereview.chromium.org/2088006/diff/28001/29005#newcode14 base/file_util_win.cc:14: #include <WinIoCtl.h> Remove this. http://codereview.chromium.org/2088006/diff/28001/29005#newcode961 base/file_util_win.cc:961: // In Vista, GetFinalPathNameByHandle() would give us the real path I'd also use FILE_SHARE_DELETE
Ready for another look. On 2010/05/18 01:18:00, rvargas wrote: > You could use CreateSymbolicLink and limit the test to Vista+ (make sure that > you can delete it afterwards), or issue a FSCTL_SET_REPARSE_POINT ioctl (copy > the code from src/sandbox/test/common/test_util.cc) I added code to create and delete junctions. > <path> is not really limited to MAX_PATH either, but in general we fail in that > case already; but yes, this code could introduce new failures due to the > expansion of any part of the path (not just the drive). > > I guess adding a few bytes to the array (and clearly documenting the limitation) > is good enough, but I would also verify the behavior of this code when the > result exceeds the size of the provided buffer (and MAX_PATH). I added 100 bytes, and added documentation in file_util.h .
http://codereview.chromium.org/2088006/diff/28001/29005 File base/file_util_win.cc (right): http://codereview.chromium.org/2088006/diff/28001/29005#newcode14 base/file_util_win.cc:14: #include <WinIoCtl.h> On 2010/05/18 01:18:00, rvargas wrote: > Remove this. Done. http://codereview.chromium.org/2088006/diff/28001/29005#newcode961 base/file_util_win.cc:961: FILE_SHARE_READ | FILE_SHARE_WRITE, On 2010/05/18 01:18:00, rvargas wrote: > I'd also use FILE_SHARE_DELETE Done.
Looks good. Just a few issues with the test. http://codereview.chromium.org/2088006/diff/45001/46004 File base/file_util_unittest.cc (right): http://codereview.chromium.org/2088006/diff/45001/46004#newcode392 base/file_util_unittest.cc:392: // To test that file_util::NormalazeFilePath() deals with NTFS junctions nit: typo http://codereview.chromium.org/2088006/diff/45001/46004#newcode395 base/file_util_unittest.cc:395: typedef struct _REPARSE_DATA_BUFFER { I'd say all this block should go into the unnamed namespace. http://codereview.chromium.org/2088006/diff/45001/46004#newcode523 base/file_util_unittest.cc:523: ::CreateFile(to_sub_a.value().c_str(), nit: extra indent http://codereview.chromium.org/2088006/diff/45001/46004#newcode537 base/file_util_unittest.cc:537: ASSERT_TRUE(file_util::CreateDirectory(to_base_b)); If this fails (or any assert after this one), you'll end up with the directory structure created, including the first reparse point. Any subsequent run of this test will have a problem 'cause the reparse point is already there. http://codereview.chromium.org/2088006/diff/45001/46004#newcode582 base/file_util_unittest.cc:582: ASSERT_FALSE(file_util::NormalizeFilePath(long_path, &normalized_path)); I don't think this is testing the right thing... what happens if you start long_path path with \??\ ?
http://codereview.chromium.org/2088006/diff/45001/46004 File base/file_util_unittest.cc (right): http://codereview.chromium.org/2088006/diff/45001/46004#newcode392 base/file_util_unittest.cc:392: // To test that file_util::NormalazeFilePath() deals with NTFS junctions On 2010/06/02 18:35:27, rvargas wrote: > nit: typo Done. http://codereview.chromium.org/2088006/diff/45001/46004#newcode395 base/file_util_unittest.cc:395: typedef struct _REPARSE_DATA_BUFFER { On 2010/06/02 18:35:27, rvargas wrote: > I'd say all this block should go into the unnamed namespace. The entire file is wrapped in an unnamed namespace, so I am not sure what you mean. I considered putting this in the anonymous namespace in file_util.cc, but then there is no way for the test to use it. I moved this block to the top of the file. Let me know if I misunderstood what you meant. http://codereview.chromium.org/2088006/diff/45001/46004#newcode523 base/file_util_unittest.cc:523: ::CreateFile(to_sub_a.value().c_str(), On 2010/06/02 18:35:27, rvargas wrote: > nit: extra indent Done. http://codereview.chromium.org/2088006/diff/45001/46004#newcode537 base/file_util_unittest.cc:537: ASSERT_TRUE(file_util::CreateDirectory(to_base_b)); On 2010/06/02 18:35:27, rvargas wrote: > If this fails (or any assert after this one), you'll end up with the directory > structure created, including the first reparse point. Any subsequent run of this > test will have a problem 'cause the reparse point is already there. FileUtilTest::SetUp() and FileUtilTest::TearDown() both remove the working directory of the test, so that each run has a clean slate. I verified that they remove the entire directory, even if a reparse point is left by the test. I could move the body of the test into a new function, so that even if an assertion fails DeleteReparsePoint runs. However, this makes the test uglier to my eye, and TearDown() already solves the problem. Your call if it is worth doing this. http://codereview.chromium.org/2088006/diff/45001/46004#newcode582 base/file_util_unittest.cc:582: ASSERT_FALSE(file_util::NormalizeFilePath(long_path, &normalized_path)); On 2010/06/02 18:35:27, rvargas wrote: > I don't think this is testing the right thing... what happens if you start > long_path path with \??\ ? Using a call to GetLastError() in NormalizePath(), I see that the file is mapped, ::GetMappedFileName() is called, and it fails with error code 1006, ERROR_FILE_INVALID. Adding L"\\??\\" as a prefix to the path does not change this. Why should an L"\\??\\" change the behavior of ::GetMappedFileName()?
http://codereview.chromium.org/2088006/diff/45001/46004 File base/file_util_unittest.cc (right): http://codereview.chromium.org/2088006/diff/45001/46004#newcode395 base/file_util_unittest.cc:395: typedef struct _REPARSE_DATA_BUFFER { On 2010/06/03 16:07:11, Sam Kerner (Chrome) wrote: > On 2010/06/02 18:35:27, rvargas wrote: > > I'd say all this block should go into the unnamed namespace. > > The entire file is wrapped in an unnamed namespace, so I am not sure what you > mean. I considered putting this in the anonymous namespace in file_util.cc, but > then there is no way for the test to use it. > > I moved this block to the top of the file. Let me know if I misunderstood what > you meant. No, this code belongs to this file. I saw the namespace, and I saw tests (that I've never seen being part of the anonymous namespace), so I thought it was not part of it. Given that, top of the file or right above the test is up to you, thanks. http://codereview.chromium.org/2088006/diff/45001/46004#newcode537 base/file_util_unittest.cc:537: ASSERT_TRUE(file_util::CreateDirectory(to_base_b)); On 2010/06/03 16:07:11, Sam Kerner (Chrome) wrote: > On 2010/06/02 18:35:27, rvargas wrote: > > If this fails (or any assert after this one), you'll end up with the directory > > structure created, including the first reparse point. Any subsequent run of > this > > test will have a problem 'cause the reparse point is already there. > > FileUtilTest::SetUp() and FileUtilTest::TearDown() both remove the working > directory of the test, so that each run has a clean slate. I verified that they > remove the entire directory, even if a reparse point is left by the test. > > I could move the body of the test into a new function, so that even if an > assertion fails DeleteReparsePoint runs. > However, this makes the test uglier to my eye, and TearDown() already solves the > problem. Your call if it is worth doing this. If TearDown can cope well with the reparse points that's great, nothing else to do!. (did you verify that the loop to_base_b is not an issue?) http://codereview.chromium.org/2088006/diff/45001/46004#newcode582 base/file_util_unittest.cc:582: ASSERT_FALSE(file_util::NormalizeFilePath(long_path, &normalized_path)); On 2010/06/03 16:07:11, Sam Kerner (Chrome) wrote: > On 2010/06/02 18:35:27, rvargas wrote: > > I don't think this is testing the right thing... what happens if you start > > long_path path with \??\ ? > > Using a call to GetLastError() in NormalizePath(), I see that the file is > mapped, ::GetMappedFileName() is called, and it fails with error code 1006, > ERROR_FILE_INVALID. Adding L"\\??\\" as a prefix to the path does not change > this. > > Why should an L"\\??\\" change the behavior of ::GetMappedFileName()? The way I see this, the name to extend is long and the result is short, so we don't go through the case of needing more than MAX_PATH for the buffer of GetMappedFileName (and that seemed like the thing to test, but the more tests the better). With that assumption, the call to CreateFile should have failed (long name not prepended with \??\ (actually, it should be \\?\ for CreateFile)) so the whole function should have returned false without even mapping the file. Adding the correct prefix should have fixed that issue and allowed the function to move on.
http://codereview.chromium.org/2088006/diff/45001/46004 File base/file_util_unittest.cc (right): http://codereview.chromium.org/2088006/diff/45001/46004#newcode395 base/file_util_unittest.cc:395: typedef struct _REPARSE_DATA_BUFFER { On 2010/06/03 18:39:06, rvargas wrote: > On 2010/06/03 16:07:11, Sam Kerner (Chrome) wrote: > > On 2010/06/02 18:35:27, rvargas wrote: > > > I'd say all this block should go into the unnamed namespace. > > > > The entire file is wrapped in an unnamed namespace, so I am not sure what you > > mean. I considered putting this in the anonymous namespace in file_util.cc, > but > > then there is no way for the test to use it. > > > > I moved this block to the top of the file. Let me know if I misunderstood > what > > you meant. > > No, this code belongs to this file. I saw the namespace, and I saw tests (that > I've never seen being part of the anonymous namespace), so I thought it was not > part of it. Given that, top of the file or right above the test is up to you, > thanks. Okay. Leaving it at the top of the file. http://codereview.chromium.org/2088006/diff/45001/46004#newcode537 base/file_util_unittest.cc:537: ASSERT_TRUE(file_util::CreateDirectory(to_base_b)); On 2010/06/03 18:39:06, rvargas wrote: > On 2010/06/03 16:07:11, Sam Kerner (Chrome) wrote: > > On 2010/06/02 18:35:27, rvargas wrote: > > > If this fails (or any assert after this one), you'll end up with the > directory > > > structure created, including the first reparse point. Any subsequent run of > > this > > > test will have a problem 'cause the reparse point is already there. > > > > FileUtilTest::SetUp() and FileUtilTest::TearDown() both remove the working > > directory of the test, so that each run has a clean slate. I verified that > they > > remove the entire directory, even if a reparse point is left by the test. > > > > I could move the body of the test into a new function, so that even if an > > assertion fails DeleteReparsePoint runs. > > However, this makes the test uglier to my eye, and TearDown() already solves > the > > problem. Your call if it is worth doing this. > > If TearDown can cope well with the reparse points that's great, nothing else to > do!. (did you verify that the loop to_base_b is not an issue?) > Yes, I added ASSERT_TRUE(0) before the reparse points are removed, and the test directory is cleaned up completely. http://codereview.chromium.org/2088006/diff/45001/46004#newcode582 base/file_util_unittest.cc:582: ASSERT_FALSE(file_util::NormalizeFilePath(long_path, &normalized_path)); On 2010/06/03 18:39:06, rvargas wrote: > On 2010/06/03 16:07:11, Sam Kerner (Chrome) wrote: > > On 2010/06/02 18:35:27, rvargas wrote: > > > I don't think this is testing the right thing... what happens if you start > > > long_path path with \??\ ? > > > > Using a call to GetLastError() in NormalizePath(), I see that the file is > > mapped, ::GetMappedFileName() is called, and it fails with error code 1006, > > ERROR_FILE_INVALID. Adding L"\\??\\" as a prefix to the path does not change > > this. > > > > Why should an L"\\??\\" change the behavior of ::GetMappedFileName()? > > The way I see this, the name to extend is long and the result is short, so we > don't go through the case of needing more than MAX_PATH for the buffer of > GetMappedFileName (and that seemed like the thing to test, but the more tests > the better). With that assumption, the call to CreateFile should have failed > (long name not prepended with \??\ (actually, it should be \\?\ for CreateFile)) > so the whole function should have returned false without even mapping the file. > Adding the correct prefix should have fixed that issue and allowed the function > to move on. Yes, I was testing the wrong case. I added a test that has a path longer than MAX_PATH wchars when expanded.
http://codereview.chromium.org/2088006/diff/59001/60004 File base/file_util_unittest.cc (right): http://codereview.chromium.org/2088006/diff/59001/60004#newcode134 base/file_util_unittest.cc:134: //ASSERT_FALSE(file_util::PathExists(test_dir_)); Oops, will need to revert this...
http://codereview.chromium.org/2088006/diff/59001/60004 File base/file_util_unittest.cc (right): http://codereview.chromium.org/2088006/diff/59001/60004#newcode134 base/file_util_unittest.cc:134: //ASSERT_FALSE(file_util::PathExists(test_dir_)); On 2010/06/04 19:18:54, Sam Kerner (Chrome) wrote: > Oops, will need to revert this... Done.
LGTM, Thanks. http://codereview.chromium.org/2088006/diff/76001/73006 File base/file_util_win.cc (right): http://codereview.chromium.org/2088006/diff/76001/73006#newcode990 base/file_util_win.cc:990: // GetMappedFileName() will fail if the result is longer than MAX_PATH. rant: I'm amused by how dumb the implementation of GetMappedFileName is :( http://codereview.chromium.org/2088006/diff/76001/73006#newcode1001 base/file_util_win.cc:1001: // On NTFS disks, GetMappedFileName will return a path that starts with nit: This is true for any file system.
rubber stamp LGTM. Thanks rvargas for the review. On 2010/06/07 18:50:16, rvargas wrote: > LGTM, Thanks. > > http://codereview.chromium.org/2088006/diff/76001/73006 > File base/file_util_win.cc (right): > > http://codereview.chromium.org/2088006/diff/76001/73006#newcode990 > base/file_util_win.cc:990: // GetMappedFileName() will fail if the result is > longer than MAX_PATH. > rant: I'm amused by how dumb the implementation of GetMappedFileName is :( > > http://codereview.chromium.org/2088006/diff/76001/73006#newcode1001 > base/file_util_win.cc:1001: // On NTFS disks, GetMappedFileName will return a > path that starts with > nit: This is true for any file system.
http://codereview.chromium.org/2088006/diff/76001/73006 File base/file_util_win.cc (right): http://codereview.chromium.org/2088006/diff/76001/73006#newcode1001 base/file_util_win.cc:1001: // On NTFS disks, GetMappedFileName will return a path that starts with On 2010/06/07 18:50:17, rvargas wrote: > nit: This is true for any file system. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
