Re: [PATCH v2 3/6] Make sure fsck_commit_buffer() does not run out of the buffer
Johannes Schindelin writes:
> So far, we assumed that the buffer is NUL terminated, but this is not
> a safe assumption, now that we opened the fsck_object() API to pass a
> buffer directly.
>
> So let's make sure that there is at least an empty line in the buffer.
> That way, our checks would fail if the empty line was encountered
> prematurely, and consequently we can get away with the current string
> comparisons even with non-NUL-terminated buffers are passed to
> fsck_object().
>
> Signed-off-by: Johannes Schindelin
> ---
> fsck.c | 23 +++
> 1 file changed, 23 insertions(+)
>
> diff --git a/fsck.c b/fsck.c
> index dd77628..9dd7d12 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -237,6 +237,26 @@ static int fsck_tree(struct tree *item, int strict,
> fsck_error error_func)
> return retval;
> }
>
> +static int require_end_of_header(const void *data, unsigned long size,
> + struct object *obj, fsck_error error_func)
> +{
> + const char *buffer = (const char *)data;
> + int i;
> +
> + for (i = 0; i < size; i++) {
> + switch (buffer[i]) {
> + case '\0':
> + return error_func(obj, FSCK_ERROR,
> + "invalid message: NUL at offset %d", i);
Isn't this "invalid header"? After all we haven't escaped this loop
and haven't seen the message part of the commit object (and it is
the same if you are going to later reuse this for tag objects).
> + case '\n':
> + if (i + 1 < size && buffer[i + 1] == '\n')
> + return 0;
> + }
> + }
> +
> + return error_func(obj, FSCK_ERROR, "invalid buffer: missing empty
> line");
I think you meant "missing end of header" here.
Other than these small nits, the patch looks good from a cursory
read.
> +}
> +
> static int fsck_ident(const char **ident, struct object *obj, fsck_error
> error_func)
> {
> char *end;
> @@ -284,6 +304,9 @@ static int fsck_commit_buffer(struct commit *commit,
> const char *buffer,
> unsigned parent_count, parent_line_count = 0;
> int err;
>
> + if (require_end_of_header(buffer, size, &commit->object, error_func))
> + return -1;
> +
> if (!skip_prefix(buffer, "tree ", &buffer))
> return error_func(&commit->object, FSCK_ERROR, "invalid format
> - expected 'tree' line");
> if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/6] Make sure fsck_commit_buffer() does not run out of the buffer
On Wed, Sep 10, 2014 at 9:52 AM, Johannes Schindelin
wrote:
> So far, we assumed that the buffer is NUL terminated, but this is not
> a safe assumption, now that we opened the fsck_object() API to pass a
> buffer directly.
>
> So let's make sure that there is at least an empty line in the buffer.
> That way, our checks would fail if the empty line was encountered
> prematurely, and consequently we can get away with the current string
> comparisons even with non-NUL-terminated buffers are passed to
> fsck_object().
>
> Signed-off-by: Johannes Schindelin
> ---
> fsck.c | 23 +++
> 1 file changed, 23 insertions(+)
>
> diff --git a/fsck.c b/fsck.c
> index dd77628..9dd7d12 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -237,6 +237,26 @@ static int fsck_tree(struct tree *item, int strict,
> fsck_error error_func)
> return retval;
> }
>
> +static int require_end_of_header(const void *data, unsigned long size,
> + struct object *obj, fsck_error error_func)
> +{
> + const char *buffer = (const char *)data;
> + int i;
> +
> + for (i = 0; i < size; i++) {
Should 'i' have type 'unsigned long', rather than 'int', to be
consistent with the type of 'size'?
> + switch (buffer[i]) {
> + case '\0':
> + return error_func(obj, FSCK_ERROR,
> + "invalid message: NUL at offset %d", i);
> + case '\n':
> + if (i + 1 < size && buffer[i + 1] == '\n')
> + return 0;
> + }
> + }
> +
> + return error_func(obj, FSCK_ERROR, "invalid buffer: missing empty
> line");
> +}
> +
> static int fsck_ident(const char **ident, struct object *obj, fsck_error
> error_func)
> {
> char *end;
> @@ -284,6 +304,9 @@ static int fsck_commit_buffer(struct commit *commit,
> const char *buffer,
> unsigned parent_count, parent_line_count = 0;
> int err;
>
> + if (require_end_of_header(buffer, size, &commit->object, error_func))
> + return -1;
> +
> if (!skip_prefix(buffer, "tree ", &buffer))
> return error_func(&commit->object, FSCK_ERROR, "invalid
> format - expected 'tree' line");
> if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')
> --
> 2.0.0.rc3.9669.g840d1f9
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/6] Make sure fsck_commit_buffer() does not run out of the buffer
Hi Junio,
On Wed, 10 Sep 2014, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> > diff --git a/fsck.c b/fsck.c
> > index dd77628..9dd7d12 100644
> > --- a/fsck.c
> > +++ b/fsck.c
> > @@ -237,6 +237,26 @@ static int fsck_tree(struct tree *item, int strict,
> > fsck_error error_func)
> > return retval;
> > }
> >
> > +static int require_end_of_header(const void *data, unsigned long size,
> > + struct object *obj, fsck_error error_func)
> > +{
> > + const char *buffer = (const char *)data;
> > + int i;
> > +
> > + for (i = 0; i < size; i++) {
> > + switch (buffer[i]) {
> > + case '\0':
> > + return error_func(obj, FSCK_ERROR,
> > + "invalid message: NUL at offset %d", i);
>
> Isn't this "invalid header"? After all we haven't escaped this loop
> and haven't seen the message part of the commit object (and it is
> the same if you are going to later reuse this for tag objects).
My reasoning for keeping it saying "message" was that a message consists
of a header and a body. I will change it to "unterminated header" instead,
also in the error message when no NUL was found.
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/6] Make sure fsck_commit_buffer() does not run out of the buffer
Johannes Schindelin writes:
>> > + for (i = 0; i < size; i++) {
>> > + switch (buffer[i]) {
>> > + case '\0':
>> > + return error_func(obj, FSCK_ERROR,
>> > + "invalid message: NUL at offset %d", i);
>>
>> Isn't this "invalid header"? After all we haven't escaped this loop
>> and haven't seen the message part of the commit object (and it is
>> the same if you are going to later reuse this for tag objects).
>
> My reasoning for keeping it saying "message" was that a message consists
> of a header and a body. I will change it to "unterminated header" instead,
> also in the error message when no NUL was found.
Because end users think of a "message" in the context of discussing
either a commit or a tag as what they give as the value to the "-m"
option, the object payload consists of a header and a body the
latter of which *is* the message to them. Choosing a word that is
not "message" is a good way to avoid potential confusion here.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
