threads / discuss / 43132

Re: sizeof(struct ...)

Subject: Re: sizeof(struct ...)

## tl;dr

13 messages between Nov 23, 2006 and Nov 24, 2006.

replies: 12people: 5as markdown or json

Gerrit Pape· Nov 23, 2006, 10:16 UTC · lore

sizeof(struct ...)

Hi, I don't think we can rely on sizeof(struct ...) to be the exact size of the struct as defined. As the selftests show, archive-zip doesn't work correctly on Debian/arm

 http://buildd.debian.org/fetch.cgi?&pkg=git-core&ver=1%3A1.4.4-1&arch=arm&stamp=1164122355&file=log

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.

Regards, Gerrit.
diff --git a/archive-zip.c b/archive-zip.c
index ae5572a..4fcda44 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -211,7 +211,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 = 46 + 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 +234,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, 46);
+	zip_dir_offset += 46;
 	memcpy(zip_dir + zip_dir_offset, path, pathlen);
 	zip_dir_offset += pathlen;
 	zip_dir_entries++;
@@ -251,8 +251,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, 30);
+	zip_offset += 30;
 	write_or_die(1, path, pathlen);
 	zip_offset += pathlen;
 	if (compressed_size > 0) {
@@ -282,7 +282,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, 22);
 	if (sha1)
 		write_or_die(1, sha1_to_hex(sha1), 40);
René Scharfe· Nov 23, 2006, 12:43 UTC · re: Gerrit Pape · lore
Gerrit Pape schrieb:
Show 10 quoted lines
> Hi, I don't think we can rely on sizeof(struct ...) to be the exact size
> of the struct as defined.  As the selftests show, archive-zip doesn't
> work correctly on Debian/arm
> 
>  http://buildd.debian.org/fetch.cgi?&pkg=git-core&ver=1%3A1.4.4-1&arch=arm&stamp=1164122355&file=log
> 
> 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?

Thanks, René

 archive-zip.c |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/archive-zip.c b/archive-zip.c
index ae5572a..4caaec4 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[0];
 };
 
 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[0];
 };
 
 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[0];
 };
 
+/*
+ * 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);
René Scharfe· Nov 23, 2006, 13:38 UTC · re: René Scharfe · lore

Oh no, whitespace damage! The major downside of getting a new machine is that Thunderbird needs to be beaten into submission again. :-( Here's the patch a second time, hopefully intact.

René
 archive-zip.c |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/archive-zip.c b/archive-zip.c
index ae5572a..4caaec4 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[0];
 };
 
 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[0];
 };
 
 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[0];
 };
 
+/*
+ * 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);
Andy Whitcroft· Nov 23, 2006, 13:55 UTC · re: René Scharfe · lore
René Scharfe wrote:
Show 6 quoted lines
> Oh no, whitespace damage!  The major downside of getting a new
> machine is that Thunderbird needs to be beaten into submission
> again. :-(  Here's the patch a second time, hopefully intact.
> 
> René
>

This structure represents an on-disk/on-the-wire thing, should we not be specifying it in some architecture neutral way? You are going to get the length right in the case of tail padding but not in the face of any other padding internally.

You see packing attributes applied to similar things in the kernel. Perhaps they are relevant here? Is there not some kind of attribute thing we can apply to this structure to prevent the padding? You see that in the kernel from time to time.

struct foo { } __attribute__((packed));

-apw
Show 87 quoted lines
> 
>  archive-zip.c |   24 ++++++++++++++++++------
>  1 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/archive-zip.c b/archive-zip.c
> index ae5572a..4caaec4 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[0];
>  };
>  
>  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[0];
>  };
>  
>  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[0];
>  };
>  
> +/*
> + * 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);
>  }
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
René Scharfe· Nov 23, 2006, 15:45 UTC · re: Andy Whitcroft · lore
Andy Whitcroft schrieb:
Show 12 quoted lines
> This structure represents an on-disk/on-the-wire thing, should we not be
> specifying it in some architecture neutral way?  You are going to get
> the length right in the case of tail padding but not in the face of any
> other padding internally.
> 
> You see packing attributes applied to similar things in the kernel.
> Perhaps they are relevant here?
> Is there not some kind of attribute thing we can apply to this structure
> to prevent the padding?  You see that in the kernel from time to time.
> 
> struct foo {
> } __attribute__((packed));

Yes, that would be nice, but unfortunately __attribute__ is no standard C. Is there really a compiler that inserts padding between arrays of unsigned chars?

Thanks,
Erik Mouw· Nov 23, 2006, 15:54 UTC · re: René Scharfe · lore
On Thu, Nov 23, 2006 at 04:45:09PM +0100, Ren? Scharfe wrote:
Show 11 quoted lines
> Andy Whitcroft schrieb:
> > You see packing attributes applied to similar things in the kernel.
> > Perhaps they are relevant here?
> > Is there not some kind of attribute thing we can apply to this structure
> > to prevent the padding?  You see that in the kernel from time to time.
> > 
> > struct foo {
> > } __attribute__((packed));
> 
> Yes, that would be nice, but unfortunately __attribute__ is no standard
> C.

There is no standard C way to pack structures. Some compilers use #pragma's, gcc uses __attribute__((packed)).

>  Is there really a compiler that inserts padding between arrays of
> unsigned chars?
Yes, that compiler is called "gcc".
#include <stdio.h>
struct foo {
        unsigned char a[3];
        unsigned char b[3];
};
int main(void)
{
        printf("%d\n", sizeof(struct foo));
        return 0;
}
On i386 that prints 6, on ARM it prints 8.
Erik
-- 
+-- Erik Mouw -- www.harddisk-recovery.com -- +31 70 370 12 90 --
René Scharfe· Nov 23, 2006, 16:14 UTC · re: Erik Mouw · lore
Erik Mouw schrieb:
Show 20 quoted lines
> On Thu, Nov 23, 2006 at 04:45:09PM +0100, René Scharfe wrote:
>>  Is there really a compiler that inserts padding between arrays of
>> unsigned chars?
> 
> Yes, that compiler is called "gcc".
> 
> #include <stdio.h>
> 
> struct foo {
>         unsigned char a[3];
>         unsigned char b[3];
> };
> 
> int main(void)
> {
>         printf("%d\n", sizeof(struct foo));
>         return 0;
> }
> 
> On i386 that prints 6, on ARM it prints 8.

Does it add 1 byte after a and and 1 after b or two after b? I suspect it's the latter case -- otherwise Gerrit's patch, which started this thread, wouldn't help solve his problem. Or the pad sizing follows complicated rules that I do not understand at the moment.

Time to look for an ARM emulator, it seems.
Thanks,
Andy Whitcroft· Nov 23, 2006, 16:19 UTC · re: René Scharfe · lore
René Scharfe wrote:
Show 28 quoted lines
> Erik Mouw schrieb:
>> On Thu, Nov 23, 2006 at 04:45:09PM +0100, René Scharfe wrote:
>>>  Is there really a compiler that inserts padding between arrays of
>>> unsigned chars?
>> Yes, that compiler is called "gcc".
>>
>> #include <stdio.h>
>>
>> struct foo {
>>         unsigned char a[3];
>>         unsigned char b[3];
>> };
>>
>> int main(void)
>> {
>>         printf("%d\n", sizeof(struct foo));
>>         return 0;
>> }
>>
>> On i386 that prints 6, on ARM it prints 8.
> 
> Does it add 1 byte after a and and 1 after b or two after b?
> I suspect it's the latter case -- otherwise Gerrit's patch,
> which started this thread, wouldn't help solve his problem.
> Or the pad sizing follows complicated rules that I do not
> understand at the moment.
> 
> Time to look for an ARM emulator, it seems.

Perhaps we can look and see what a portable application like gzip or bzip2 do in this situation. They must have the same problem.

René Scharfe· Nov 23, 2006, 17:57 UTC · re: Andy Whitcroft · lore
Andy Whitcroft schrieb:
> Perhaps we can look and see what a portable application like gzip or
> bzip2 do in this situation.  They must have the same problem.

Info-ZIP's zip uses structs only for in-memory storage and has a write function for each of them that writes the members one by one. I find the structs in archive-zip.c easier to read, but I might be biased. ;-)

Anyway, archive-zip.c assumes that there is no padding between unsigned char arrays and that an unsigned char is exactly one byte wide. The additional current assumption -- that sizeof(struct ...) sums up the sizes of all struct members -- is wrong on ARM, and the patches in this thread correct this error.

So we're not as portable as Info-ZIP, but I think the assumptions above hold true for all interesting architectures. And we have a readable description of the on-disk ZIP file headers.

Erik Mouw· Nov 23, 2006, 16:42 UTC · re: René Scharfe · lore
On Thu, Nov 23, 2006 at 05:14:44PM +0100, Ren? Scharfe wrote:
Show 27 quoted lines
> Erik Mouw schrieb:
> > On Thu, Nov 23, 2006 at 04:45:09PM +0100, Ren? Scharfe wrote:
> >>  Is there really a compiler that inserts padding between arrays of
> >> unsigned chars?
> > 
> > Yes, that compiler is called "gcc".
> > 
> > #include <stdio.h>
> > 
> > struct foo {
> >         unsigned char a[3];
> >         unsigned char b[3];
> > };
> > 
> > int main(void)
> > {
> >         printf("%d\n", sizeof(struct foo));
> >         return 0;
> > }
> > 
> > On i386 that prints 6, on ARM it prints 8.
> 
> Does it add 1 byte after a and and 1 after b or two after b?
> I suspect it's the latter case -- otherwise Gerrit's patch,
> which started this thread, wouldn't help solve his problem.
> Or the pad sizing follows complicated rules that I do not
> understand at the moment.
You're right, it adds the padding after b:

#define offsetof(TYPE, MEMBER) __builtin_offsetof (TYPE, MEMBER) printf("%d %d\n", offsetof(struct foo, a), offsetof(struct foo, b));

prints "0 3" on ARM.
> Time to look for an ARM emulator, it seems.

objdump -D -S is your friend. I didn't have an ARM target ready, but at least I know enough ARM assembly that I can see what it will print :)

Erik
-- 
+-- Erik Mouw -- www.harddisk-recovery.com -- +31 70 370 12 90 --
Junio C Hamano· Nov 23, 2006, 20:47 UTC · re: René Scharfe · lore
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
Show 17 quoted lines
> Gerrit Pape schrieb:
>...
>> 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.

René Scharfe· Nov 23, 2006, 22:02 UTC · re: Junio C Hamano · lore

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

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 <rene.scharfe@lsrfire.ath.cx>
 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);
 }
Gerrit Pape· Nov 24, 2006, 08:53 UTC · re: René Scharfe · lore

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

On Thu, Nov 23, 2006 at 11:02:37PM +0100, Ren? Scharfe wrote:
Show 18 quoted lines
> 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 <rene.scharfe@lsrfire.ath.cx>

← back to recent threads