> On Dec. 15, 2015, 4:24 a.m., Alex Naparu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/dirent.hpp, > > line 79 > > <https://reviews.apache.org/r/39019/diff/8/?file=1128789#file1128789line79> > > > > Consider using a smart pointer instead. Same below.
I would love to use a smart pointer, but unfortunately, the goal here is to make a drop-in replacement for the POSIX `dirent` API, which is in raw C. > On Dec. 15, 2015, 4:24 a.m., Alex Naparu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/dirent.hpp, > > line 215 > > <https://reviews.apache.org/r/39019/diff/8/?file=1128789#file1128789line215> > > > > This is copied here, but malloc'd elsewhere. Is that safe? I'm no C expert by any stretch of the imagination, but I believe this is safe. The reason is, we're actually not `malloc`'ing that array at all, we're `strcpy`'ing a path into a stack-allocated array of `MAX_PATH` in length. According to [1], this includes null terminator, so I think this is ok to do. [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx > On Dec. 15, 2015, 4:24 a.m., Alex Naparu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/dirent.hpp, > > line 230 > > <https://reviews.apache.org/r/39019/diff/8/?file=1128789#file1128789line230> > > > > Consider using safe versions of string functions I'm not an expert in C or Windows, but looking at this, it seems like we can't use `strncpy` or `strcpy_s` for the potentially-insecure call to `strcpy` in `opendir` (since we don't know the length of `path` ahead of time), and for the calls to `strcpy` that we can use these functions, it's not helpful, because they are `strcpy`'ing stuff from Windows API calls like `FindNextFile`, which should be outputting something sensible anyway. So, it seems like this is probably not worth changing? What do you think? > On Dec. 15, 2015, 4:24 a.m., Alex Naparu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp, line 74 > > <https://reviews.apache.org/r/39019/diff/8/?file=1128790#file1128790line74> > > > > Might want to add this as a reference too: > > https://msdn.microsoft.com/en-us/library/930f87yf.aspx > > > > MAX_PATH is horribly obsolete, we might want to consider checking > > whether we're running on NTFS for all FS-related ops. That will be true in > > the vast majority of cases these days. I actually expect that we will probably mostly never run on FAT at all, but I wasn't super well versed in how to get the Windows API to not use the FAT limitations, so we went with mostly FAT-limited APIs, because they seemed simpler. That said, I'm not an expert at either Windows or C++, so maybe this was all just naive. - Alex ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39019/#review110392 ----------------------------------------------------------- On Nov. 17, 2015, 7:06 p.m., Alex Clemmer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39019/ > ----------------------------------------------------------- > > (Updated Nov. 17, 2015, 7:06 p.m.) > > > Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph > Wu. > > > Bugs: MESOS-3441 > https://issues.apache.org/jira/browse/MESOS-3441 > > > Repository: mesos > > > Description > ------- > > Windows: Added dirent compat code for non-Unix systems. > > > Diffs > ----- > > > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/dirent.hpp > PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp > 1a7037d64afeedc340258c92067e95d1d3caa027 > > Diff: https://reviews.apache.org/r/39019/diff/ > > > Testing > ------- > > `make check` from autotools on Ubuntu 15. > `make check` from CMake on OS X 10.10. > Ran `check` project in VS on Windows 10. > > > Thanks, > > Alex Clemmer > >
