> 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 > >
