From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 06 Aug 2012 05:17:22 GMT
Subject: Re: [PATCH/RFC v2 09/16] Read index-v5
Message-ID: <7v4nog1twd.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1344203353-2819-10-git-send-email-t.gummerer@gmail.com>

Thomas Gummerer <t.gummerer@gmail.com> writes:

> +static struct directory_entry *read_directories_v5(unsigned int *dir_offset,
> +				unsigned int *dir_table_offset,
> +				void *mmap,
> +				int mmap_size)
> +{
> +	int i, ondisk_directory_size;
> +	uint32_t *filecrc, *beginning, *end;
> +	struct directory_entry *current = NULL;
> +	struct ondisk_directory_entry *disk_de;
> +	struct directory_entry *de;
> +	unsigned int data_len, len;
> +	char *name;
> +
> +	ondisk_directory_size = sizeof(disk_de->flags)
> +		+ sizeof(disk_de->foffset)
> +		+ sizeof(disk_de->cr)
> +		+ sizeof(disk_de->ncr)
> +		+ sizeof(disk_de->nsubtrees)
> +		+ sizeof(disk_de->nfiles)
> +		+ sizeof(disk_de->nentries)
> +		+ sizeof(disk_de->sha1);
> +	name = (char *)mmap + *dir_offset;
> +	beginning = mmap + *dir_table_offset;

Notice how you computed name with pointer arithmetic by first
casting mmap (which is "void *") and when computing beginning, you
forgot to cast mmap and attempted pointer arithmetic with "void *".
The latter does not work and breaks compilation.

The pointer-arith with "void *" is not limited to this function.

Please check the a band-aid (I wouldn't call it a fix-up) patch I
added on top of the series before queuing the topic to 'pu'; it is
primarily to illustrate the places I noticed that have this issue.

I do not necessarily suggest that the way the band-aid patch makes
it compile is the best approach.  It might be cleaner to use a saner
type like "char *" (or perhaps "const char *") as the type to point
at a piece of memory you read from the disk.  I haven't formed an
opinion.

Thanks.