> 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!
> 
> Jie Yu wrote:
>     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.

I checked out the IRC logs; that makes sense! I reverted back to passing `this`.


- Greg


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


On Nov. 19, 2015, 11:41 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39988/
> -----------------------------------------------------------
> 
> (Updated Nov. 19, 2015, 11:41 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