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

Yes I mentioned this in IRC :P On Jul 19, 2012 8:55 PM, "Allan McRae" <allan@archlinux.org> wrote:

On 20/07/12 10:47, Dave Reisner wrote:

...

Not necessary If you zero out the line size in the !eol case when len == 0

Ah... adding:

--- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -1025,6 +1025,7 @@ int _alpm_archive_fgets(struct archive *a, struct archive_ * returned on next call */ if(len == 0) { b->line_offset[0] = '\0'; + b->real_line_size = 0; return ARCHIVE_OK; } }

all is good.