Re: [PATCH v2 2/3] textconv: support for blame

Clément Poulain [off-list ref] writes:

On Mon, 14 Jun 2010 13:40:21 -0700, Junio C Hamano [off-list ref]
wrote:

quoted

Axel Bonnet [off-list ref] writes:

quoted

@@ -86,16 +87,49 @@ struct origin {

...
+static void fill_origin_blob(struct diff_options *opt,
+			     struct origin *o, mmfile_t *file)
 {
 	if (!o->file.ptr) {
 		enum object_type type;
 		num_read_blob++;
-		file->ptr = read_sha1_file(o->blob_sha1, &type,
-					   (unsigned long *)(&(file->size)));
+
+		if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&
+		    textconv_object(o->path, o->blob_sha1, &file->ptr,
+				    (size_t *) &file->size))

This cast is not correct, as there is no guarantee that your size_t and
typeof(mmfile_t.size) are compatible.  Depending on the gcc version, you
would get "dereferencing type-punned pointer will break strict-aliasing
rules" error.

The same issue exists in Clément's patch to builtin/cat-file.c.

We did this way because we found a similar cast in prep_temp_blob(),
diff.c:

	if (convert_to_working_tree(path,
			(const char *)blob, (size_t)size, &buf)) {

where size is an unsigned long.

That is a completely different kind of cast that is sane.  The function
takes a _value_ of type size_t.

Your cast is "This function wants to store a value to a _memory location_
that is supposed to hold a value of type size_t, and I know &(file->size)
is not such a location (it is to hold a value of type 'unsigned long');
please pretend that these two distinct pointers are compatible", which is
a big no-no.