> On Nov. 13, 2015, 11:38 p.m., Jie Yu wrote:
> > src/master/http.cpp, line 853
> > <https://reviews.apache.org/r/39988/diff/6/?file=1125444#file1125444line853>
> >
> >     This looks problematic to me. 'this' will become invalid once this 
> > function returns. That means when `_reserve` is actually called after the 
> > authorization is done, it might be a dangling pointer.
> >     
> >     Instead, you can try to copy the http object (something like below):
> >     
> >     ```
> >     lambda::function<Future<Response>(bool)> _reserve =
> >       lambda::bind(
> >           &Master::Http::_operation,
> >           *this,
> >           slaveId,
> >           resources.flatten(),
> >           operation,
> >           lambda::_1);
> >     
> >     return master->authorizeReserveResources(operation.reserve(), principal)
> >       .then(defer(master->self(), _reserve));
> >     ```
> 
> Greg Mann wrote:
>     Good catch, thanks!

Mpark reached out to me about this issue. Seems that this extra copying is not 
necessary (my bad). The 'http' object will be bounded in the lambda function 
and stored in Master's libprocess handlers map.


- Jie


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


On Nov. 16, 2015, 11:53 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39988/
> -----------------------------------------------------------
> 
> (Updated Nov. 16, 2015, 11:53 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
>     https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added authorization for dynamic reservation master endpoints.
> Note: this review is continued from https://reviews.apache.org/r/37126/
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 1c4f1406f5d917f5d655a7d61d311365f8999ce0 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/tests/reservation_endpoints_tests.cpp 
> 1552e4537c4f4d79bfa4bc17ccab2df630bc32a4 
> 
> Diff: https://reviews.apache.org/r/39988/diff/
> 
> 
> Testing
> -------
> 
> This is the fourth in a chain of 5 patches. Added new reservation endpoints 
> tests to validate authorization of reserve and unreserve operations using 
> ACLs. `make check` was run to test after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Reply via email to