> On Jan. 9, 2016, 12:42 a.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/dirent.hpp,
> >  lines 61-62
> > <https://reviews.apache.org/r/39019/diff/10/?file=1175210#file1175210line61>
> >
> >     I don't see the check for pathSize wrt. MAX_PATH.

You're right that this should be there, just because it is good defensive style.

But, just for my own education: do you see a problem that I do not? There are 
known-problem overflows (like the calls to `strcat`), but they are innate to 
the API, because we don't know the length of `path` ahead of time, and 
therefore they don't go away if we add a `MAX_PATH` check. Or perhaps I am 
missing something here?


> On Jan. 9, 2016, 12:42 a.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/dirent.hpp,
> >  line 214
> > <https://reviews.apache.org/r/39019/diff/10/?file=1175210#file1175210line214>
> >
> >     are `null-terminated` right?
> >     Same for the commeint in `_reentrantAdvanceDirStream`

Good question. The point I'm actually trying to make is that 
`directory->curr.d_name` can be a statically-sized array of `MAX_PATH` size 
because `directory->fd.cFileName` is also a statically sized array of 
`MAX_PATH` size.

Let's update the comments so that this is more clear.


> On Jan. 9, 2016, 12:42 a.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/dirent.hpp,
> >  lines 193-199
> > <https://reviews.apache.org/r/39019/diff/10/?file=1175210#file1175210line193>
> >
> >     `free` is meant to be safe for `NULL/nullptr`.
> >     Why not follow this pattern?
> >     asserting `directory != NULL` seems like it might surprise people?

Well, you are the C++ programmer, so I'll trust your judgement on the `assert`s 
here.

But, since this question about `assert` comes up 3 times in your review, I'll 
answer your question, and justify the switch. I initially chose to do it this 
way because I think internal APIs are easier to reason about if all the 
functions assume the data in non-`_OUT_` params is fully initialized before you 
call. I just used `assert` to make this assumption explicit, which I think is a 
good way of broadcasting to internal users what the contract is.

Of course, (1) if you think this is less clear, then my thesis is clearly 
wrong, and (2) the actual difference in this case is not really a substantial 
amount of code/complexity, so the cost of switching to your model is not 
prohibitive, so if I were to advocate staying it would be for purely religious 
reasons.

Therefore, we shall transition to fit your suggestion.


- Alex


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39019/#review113576
-----------------------------------------------------------


On Dec. 23, 2015, 6:44 a.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39019/
> -----------------------------------------------------------
> 
> (Updated Dec. 23, 2015, 6:44 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, 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/Makefile.am 
> b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
>   
> 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/dirent.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
> d46e262e0fd1c2de36f3bf19d8bd693c23bf58cd 
> 
> 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
> 
>

Reply via email to