20 Jul
2012
20 Jul
'12
2:47 a.m.
Not necessary If you zero out the line size in the !eol case when len == 0
On Jul 19, 2012 8:44 PM, "Allan McRae" <allan@archlinux.org> wrote:
> On 20/07/12 00:54, Dave Reisner wrote:
> > 74274b5dc347ba70 which added the real_line_size to the buffer struct
> > didn't properly account for what happens when archive_fgets has to loop
> > more than once to find the end of a line. In most cases, this isn't a
> > problem, but could potentially cause a longer line such as PGP signature
> > to be improperly read.
> >
> > This patch fixes the oversight and focuses on only calculating the line
> > length when we hit the end of line marker. The effective length is then
> > calculated via pointer arithmetic as:
> >
> > (start_of_last_read + read_length) - start_of_line
> >
> > Signed-off-by: Dave Reisner <dreisner@archlinux.org>
> > ---
> > Allan noticed that PHP was reporting an invalid sig and bisected it back
> to me,
> > (of course). This was lucky enough to only affect about .1% of the sync
> DB, and
> > mostly the PGPSIG field, naturally due to it being the largest field in
> the DB
> > by a wide margin.
> >
> > lib/libalpm/util.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
> > index c2b5d44..721125a 100644
> > --- a/lib/libalpm/util.c
> > +++ b/lib/libalpm/util.c
> > @@ -1011,15 +1011,16 @@ int _alpm_archive_fgets(struct archive *a,
> struct archive_read_buffer *b)
> > }
> >
> > if(eol) {
> > - size_t len = b->real_line_size = (size_t)(eol -
> b->block_offset);
> > + size_t len = (size_t)(eol - b->block_offset);
> > memcpy(b->line_offset, b->block_offset, len);
> > b->line_offset[len] = '\0';
> > b->block_offset = eol + 1;
> > + b->real_line_size = b->line_offset + len - b->line;
> > /* this is the main return point; from here you
> can read b->line */
> > return ARCHIVE_OK;
> > } else {
> > /* we've looked through the whole block but no
> newline, copy it */
> > - size_t len = b->real_line_size = (size_t)(b->block
> + b->block_size - b->block_offset);
> > + size_t len = (size_t)(b->block + b->block_size -
> b->block_offset);
> > memcpy(b->line_offset, b->block_offset, len);
> > b->line_offset += len;
> > b->block_offset = b->block + b->block_size;
>
> Just for the record... here is my version which does not break pactests:
>
>
> diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
> index 0db9e87..e91460e 100644
> --- a/lib/libalpm/util.c
> +++ b/lib/libalpm/util.c
> @@ -947,6 +947,7 @@ int _alpm_archive_fgets(struct archive *a, struct
> archive_read_buffer *b)
> {
> /* ensure we start populating our line buffer at the beginning */
> b->line_offset = b->line;
> + b->real_line_size = 0;
>
> while(1) {
> size_t block_remaining;
> @@ -1008,7 +1009,8 @@ int _alpm_archive_fgets(struct archive *a, struct
> archive_read_buffer *b)
> }
>
> if(eol) {
> - size_t len = b->real_line_size = (size_t)(eol -
> b->block_offset);
> + size_t len = (size_t)(eol - b->block_offset);
> + b->real_line_size += len;
> memcpy(b->line_offset, b->block_offset, len);
> b->line_offset[len] = '\0';
> b->block_offset = eol + 1;
> @@ -1016,7 +1018,8 @@ int _alpm_archive_fgets(struct archive *a, struct
> archive_read_buffer *b)
> return ARCHIVE_OK;
> } else {
> /* we've looked through the whole block but no
> newline, copy it */
> - size_t len = b->real_line_size = (size_t)(b->block
> + b->block_size -
> b->block_offset);
> + size_t len = (size_t)(b->block + b->block_size -
> b->block_offset);
> + b->real_line_size += len;
> memcpy(b->line_offset, b->block_offset, len);
> b->line_offset += len;
> b->block_offset = b->block + b->block_size;
>
>
>
>
>
>