mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Klues <klue...@gmail.com>
Subject Re: Review Request 47481: Rewrote os::read() to be friendlier to reading binary data.
Date Thu, 19 May 2016 13:21:53 GMT


> On May 19, 2016, 9:12 a.m., Jan Schlicht wrote:
> > 3rdparty/stout/include/stout/os/read.hpp, line 136
> > <https://reviews.apache.org/r/47481/diff/5/?file=1386391#file1386391line136>
> >
> >     s/delete/delete[]
> >     
> >     Why not use a `std::vector<char>` for `buffer`?
> >     
> >     I've opened https://reviews.apache.org/r/47585/ to fix this.

I agree that this needs to be `delete[]`. Thanks for that.

I can see why this bug was introduced over time though. If you look back at revision 1 of
this patch, I wasn't using temporary storage at all, which would have avoided this bug altogether.
Unfortunately, this approach was vetoed by the reviewer, leading to revision 2, which used
a stack allocated array. This also didn't have the bug, but introduced other "stylistic" problems
that the reviewers weren't happy with. In revision 3, I used a `std::vector<char>` as
you suggest, but it never made it back to reviewboard because it was vetoed in external feedback.
As a compromise, I started using a `unique_ptr<>` in revision 4, but this was vetoed
as well. Looks like I overlooked the proper delete to use after 5 revisions through a very
simple patch.

I would personally do away with the temporary storage and go with the approach I have in revision
1 on review board. If we are going the temporary storage route, I  would prefer the `std::vector<char>`
approach as you suggest. 

Need to convince a shepherd though.


- Kevin


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


On May 18, 2016, 3:25 a.m., Kevin Klues wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47481/
> -----------------------------------------------------------
> 
> (Updated May 18, 2016, 3:25 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joris Van Remoortere, and switched to 'mcypark'.
> 
> 
> Bugs: MESOS-5398
>     https://issues.apache.org/jira/browse/MESOS-5398
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The previous read() implementation was based on calling getline() to
> read in chunks of data from a file. This is fine for text-based files,
> but is a little strange for binary files.
> 
> The new implementation reads in chunks of raw bytes into a stack
> allocated buffer before copying them into their final location. I
> usually don't like stack allocated buffers because of their potential
> security implications when it comes to stack buffer overflows, but I
> took extra care here to make sure there won't be any.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/os/read.hpp e1e97c1bcb7493a734fc77721a83c230b1a23724 
> 
> Diff: https://reviews.apache.org/r/47481/diff/
> 
> 
> Testing
> -------
> 
> make check -j
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message