Re: sizeof(struct ...)

3 messages, 3 authors, 2016-08-11 · open the first message on its own page

Re: sizeof(struct ...)

From: Junio C Hamano <hidden>
Date: 2016-08-11 19:24:47

René Scharfe [off-list ref] writes:

quoted hunk

Gerrit Pape schrieb:
...

quoted

It's because sizeof(struct zip_local_header) is 32, zip_dir_header 48,
and zip_dir_trailer 24, breaking the zip files.  Compiling with
-fpack-struct seemed to break other things, so I for now I ended up with
this (not so nice) workaround.

Hm, yes, this use sizeof() is not strictly correct.  But I'd very much
like to keep being lazy and let the compiler to do the summing.  How
about this patch instead?  Does it work for you, Gerrit?
...

@@ -35,6 +35,7 @@ struct zip_local_header {

	unsigned char size[4];
	unsigned char filename_length[2];
	unsigned char extra_length[2];
+	unsigned char _end[0];
};

While I think relying on the compiler to add no pad in the
middle of the structure that has only unsigned char array
members, making the compiler to sum the member length using
offsetof() is a reasonable approach to avoid hardcoding the size
of on-disk structure representation, zero-length member is not
portable.

We need to deal with tail padding in any case, and this _end[N]
is essentially a manual tail padding when N > 0, so I think that
the code should work even if you change these to _end[1], and
that would be a reasonably clean solution to this problem.

[PATCH] archive-zip: don't use sizeof(struct ...)

From: René Scharfe <hidden>
Date: 2016-08-11 19:32:30

We can't rely on sizeof(struct zip_*) returning the sum of
all struct members.  At least on ARM padding is added at the
end, as Gerrit Pape reported.  This fixes the problem but
still lets the compiler do the summing by introducing
explicit padding at the end of the structs and then taking
its offset as the combined size of the preceding members.

As Junio correctly notes, the _end[] marker array's size
must be greater than zero for compatibility with compilers
other than gcc.  The space wasted by the markers can safely
be neglected because we only have one instance of each
struct, i.e. in sum 3 wasted bytes on i386, and 0 on ARM. :)

We still rely on the compiler to not add padding between the
struct members, but that's reasonable given that all of them
are unsigned char arrays.

Signed-off-by: Rene Scharfe <redacted>


 archive-zip.c |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/archive-zip.c b/archive-zip.c
index ae5572a..36e922a 100644
--- a/archive-zip.c
+++ b/archive-zip.c

@@ -35,6 +35,7 @@ struct zip_local_header {
 	unsigned char size[4];
 	unsigned char filename_length[2];
 	unsigned char extra_length[2];
+	unsigned char _end[1];
 };
 
 struct zip_dir_header {

@@ -55,6 +56,7 @@ struct zip_dir_header {
 	unsigned char attr1[2];
 	unsigned char attr2[4];
 	unsigned char offset[4];
+	unsigned char _end[1];
 };
 
 struct zip_dir_trailer {

@@ -66,8 +68,18 @@ struct zip_dir_trailer {
 	unsigned char size[4];
 	unsigned char offset[4];
 	unsigned char comment_length[2];
+	unsigned char _end[1];
 };
 
+/*
+ * On ARM, padding is added at the end of the struct, so a simple
+ * sizeof(struct ...) reports two bytes more than the payload size
+ * we're interested in.
+ */
+#define ZIP_LOCAL_HEADER_SIZE	offsetof(struct zip_local_header, _end)
+#define ZIP_DIR_HEADER_SIZE	offsetof(struct zip_dir_header, _end)
+#define ZIP_DIR_TRAILER_SIZE	offsetof(struct zip_dir_trailer, _end)
+
 static void copy_le16(unsigned char *dest, unsigned int n)
 {
 	dest[0] = 0xff & n;

@@ -211,7 +223,7 @@ static int write_zip_entry(const unsigne
 	}
 
 	/* make sure we have enough free space in the dictionary */
-	direntsize = sizeof(struct zip_dir_header) + pathlen;
+	direntsize = ZIP_DIR_HEADER_SIZE + pathlen;
 	while (zip_dir_size < zip_dir_offset + direntsize) {
 		zip_dir_size += ZIP_DIRECTORY_MIN_SIZE;
 		zip_dir = xrealloc(zip_dir, zip_dir_size);

@@ -234,8 +246,8 @@ static int write_zip_entry(const unsigne
 	copy_le16(dirent.attr1, 0);
 	copy_le32(dirent.attr2, attr2);
 	copy_le32(dirent.offset, zip_offset);
-	memcpy(zip_dir + zip_dir_offset, &dirent, sizeof(struct zip_dir_header));
-	zip_dir_offset += sizeof(struct zip_dir_header);
+	memcpy(zip_dir + zip_dir_offset, &dirent, ZIP_DIR_HEADER_SIZE);
+	zip_dir_offset += ZIP_DIR_HEADER_SIZE;
 	memcpy(zip_dir + zip_dir_offset, path, pathlen);
 	zip_dir_offset += pathlen;
 	zip_dir_entries++;

@@ -251,8 +263,8 @@ static int write_zip_entry(const unsigne
 	copy_le32(header.size, uncompressed_size);
 	copy_le16(header.filename_length, pathlen);
 	copy_le16(header.extra_length, 0);
-	write_or_die(1, &header, sizeof(struct zip_local_header));
-	zip_offset += sizeof(struct zip_local_header);
+	write_or_die(1, &header, ZIP_LOCAL_HEADER_SIZE);
+	zip_offset += ZIP_LOCAL_HEADER_SIZE;
 	write_or_die(1, path, pathlen);
 	zip_offset += pathlen;
 	if (compressed_size > 0) {

@@ -282,7 +294,7 @@ static void write_zip_trailer(const unsi
 	copy_le16(trailer.comment_length, sha1 ? 40 : 0);
 
 	write_or_die(1, zip_dir, zip_dir_offset);
-	write_or_die(1, &trailer, sizeof(struct zip_dir_trailer));
+	write_or_die(1, &trailer, ZIP_DIR_TRAILER_SIZE);
 	if (sha1)
 		write_or_die(1, sha1_to_hex(sha1), 40);
 }

Re: [PATCH] archive-zip: don't use sizeof(struct ...)

From: Gerrit Pape <hidden>
Date: 2016-08-11 19:50:28

On Thu, Nov 23, 2006 at 11:02:37PM +0100, Ren? Scharfe wrote:

We can't rely on sizeof(struct zip_*) returning the sum of
all struct members.  At least on ARM padding is added at the
end, as Gerrit Pape reported.  This fixes the problem but
still lets the compiler do the summing by introducing
explicit padding at the end of the structs and then taking
its offset as the combined size of the preceding members.

As Junio correctly notes, the _end[] marker array's size
must be greater than zero for compatibility with compilers
other than gcc.  The space wasted by the markers can safely
be neglected because we only have one instance of each
struct, i.e. in sum 3 wasted bytes on i386, and 0 on ARM. :)

We still rely on the compiler to not add padding between the
struct members, but that's reasonable given that all of them
are unsigned char arrays.

Signed-off-by: Rene Scharfe <redacted>

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help