> -----Original Message-----
> From: Stefan Sperling [mailto:stsp_at_elego.de]
> Sent: dinsdag 22 september 2015 10:40
> To: dev_at_subversion.apache.org
> Subject: Re: svn commit: r1704374 - in /subversion/trunk/subversion:
> include/svn_diff.h include/svn_error_codes.h libsvn_diff/binary_diff.c
> libsvn_diff/diff.h libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
>
> On Mon, Sep 21, 2015 at 05:27:18PM -0000, rhuijben_at_apache.org wrote:
> > Author: rhuijben
> > Date: Mon Sep 21 17:27:11 2015
> > New Revision: 1704374
> >
> > URL: http://svn.apache.org/viewvc?rev=1704374&view=rev
> > Log:
> > Complete the parsing routines for parsing git-like binary blobs. This
patch
> > completes the parsing, but leaves out the patch application code as that
> > needs a bit more work before committing.
>
> > + while (base85_len)
> > + {
> > + unsigned info = 0;
>
> Please use an explicit integer type.
> 'unsigned int', or 'unsigned long', etc.
How is that more explicit?
Explicit would be strict 32 bit or strict 64 bit, which changing to those
types isn't.
Adding 'int' is just syntactic sugar but doesn't make it explicit.
>
> > + apr_ssize_t i, n;
> > +
> > + for (i = 0; i < 5; i++)
> > + {
> > + int value;
> > +
> > + SVN_ERR(base85_value(&value, base85_data[i]));
> > + info *= 85;
>
> What happens if 'info' overflows here?
Probably something similar to the git implementation... different final
output; most likely 100% identical on all systems. Something that would
happen on every unexpected bad value in a diff file.
>
> > + info += value;
> > + }
> > +
> > + for (i = 0, n=24; i < 4; i++, n-=8)
> > + {
> > + if (i < output_len)
> > + output_data[i] = (info >> n) & 0xFF;
> > + }
> > +
> > + base85_data += 5;
> > + base85_len -= 5;
> > + output_data += 4;
> > + output_len -= 4;
> > + }
> > +
> > + return SVN_NO_ERROR;
>
> > + while (remaining && (b85b->buf_size > b85b->buf_pos
> > + || b85b->next_pos < b85b->end_pos))
> > + {
> > + svn_stringbuf_t *line;
> > + svn_boolean_t at_eof;
> > +
> > + apr_size_t available = b85b->buf_size - b85b->buf_pos;
> > + if (available)
> > + {
> > + apr_size_t n = (remaining < available) ? remaining :
available;
> > +
> > + memcpy(dest, b85b->buffer + b85b->buf_pos, n);
> > + dest += n;
> > + remaining -= n;
>
> What if 'remaining' becomes negative here?
n = MIN(remaining, available), where all values are unsigned.
So that is not possible.
>
> > + b85b->buf_pos += n;
> > +
> > + if (!remaining)
> > + return SVN_NO_ERROR; /* *len = OK */
> > + }
> > +
Received on 2015-09-22 11:19:02 CEST