From: Piotr <pp...@si...> - 2002-09-29 22:20:37
|
Hi, I could not reach Rasterman via email, so I will post them here - I hope this is appropriate place for that. --- imlib2-1.0.6.orig/src/dynamic_filters.c Wed Apr 10 21:11:54 2002 +++ imlib2-1.0.6/src/dynamic_filters.c Mon Sep 9 09:21:11 2002 @@ -146,12 +146,12 @@ /* loader dir */ char **__imlib_ListFilters(int *num_ret) { - char **list = NULL, **l, *s, *home; + char **list = NULL, **l, *s; int num, i, pi = 0; *num_ret = 0; /* same for system loader path */ - s = (char *) realloc(s, sizeof(SYS_LOADERS_PATH) + 7 + 1); + s = (char *) malloc(sizeof(SYS_LOADERS_PATH) + 7 + 1); sprintf(s, SYS_LOADERS_PATH "/filter"); #ifndef __EMX__ l = __imlib_FileDir(s, &num); @@ -174,7 +174,6 @@ } __imlib_FileFreeDirList(l, num); } - free(home); free(s); /* List currently contains *everything in there* we need to weed out --- imlib2-1.0.6.orig/loaders/loader_tga.c Thu Oct 25 00:50:02 2001 +++ imlib2-1.0.6/loaders/loader_tga.c Tue Sep 10 00:13:33 2002 @@ -65,8 +65,6 @@ } tga_header; typedef struct { - unsigned int extensionAreaOffset; - unsigned int developerDirectoryOffset; char signature[16]; char dot; char null; The first patch is self-evident, there is a fix for realloc() of uninitialized variable, and free() of uninitialized/unused variable, which can cause bad crashes when using dynamic filters. The second patch needs more explanation. I have removed these two fields from tga_footer struct because they: - can cause compiler to add padding bytes to the struct, which will make the loader read wrong part of image file and fail to recognize TGA signature - are not used anyway I could use __attribute__ ((packed)) to tell the compiler not to use any padding bytes, but this is GNU C specific, and there were always problems with bus errors on architectures that require some types to be properly aligned. -- Piotr Pawłow mailto:pp...@si... | homepage: http://pp.siedziba.pl |
From: Michael J. <e-...@ka...> - 2002-11-06 04:15:26
|
On Monday, 30 September 2002, at 00:19:15 (+0200), Piotr Paw?ow wrote: > The first patch is self-evident, there is a fix for realloc() of > uninitialized variable, and free() of uninitialized/unused variable, > which can cause bad crashes when using dynamic filters. That was fixed ages ago. > The second patch needs more explanation. I have removed these two fields > from tga_footer struct because they: > > - can cause compiler to add padding bytes to the struct, which will make > the loader read wrong part of image file and fail to recognize TGA > signature > - are not used anyway > > I could use __attribute__ ((packed)) to tell the compiler not to use any > padding bytes, but this is GNU C specific, and there were always problems > with bus errors on architectures that require some types to be properly > aligned. Sorry, this breaks TGA loading on my system. The existing loader works fine. Michael -- Michael Jennings (a.k.a. KainX) http://www.kainx.org/ <me...@ka...> n+1, Inc., http://www.nplus1.net/ Author, Eterm (www.eterm.org) ----------------------------------------------------------------------- "For the life of me I cannot remember what made us think that we were wise and we'd never comprimise. For the life of me I cannot believe we'd ever die for these sins. We were merely freshman." -- The Verve Pipe, "The Freshman" |
From: Piotr <pp...@si...> - 2002-11-06 14:38:08
|
On Wednesday 06 November 2002 05:15, Michael Jennings wrote: > > The first patch is self-evident, there is a fix for realloc() of > That was fixed ages ago. Oh, now I see it, 6 months ago!! I am terribly sorry, I had probably=20 looked into wrong directory :(( > > The second patch needs more explanation. I have removed these two > > fields from tga_footer struct because they: > Sorry, this breaks TGA loading on my system. The existing loader > works fine. But here I am puzzled. I use gcc 2.95 to compile this loader. If=20 compiled without optimizations it works fine. When I turn on=20 optimizations here is what happens: gcc adds padding bytes at the end=20 of the tga_footer typedef struct {=20 unsigned int extensionAreaOffset;=20 unsigned int developerDirectoryOffset;=20 char signature[16];=20 char dot;=20 char null;=20 // here padding bytes are added, so when you place another struct=20 after this one ints are nicely aligned } tga_footer;=20 Because size of the struct changes, wrong part of the file is read=20 into tga_footer struct: /* read the footer first */ fseek (fp, 0L - (sizeof (tga_footer)), SEEK_END); if (fread (&footer, sizeof (tga_footer), 1, fp) !=3D 1) sizeof(tga_footer) is bigger than the real footer size in file, so it=20 seeks too far and wrong part of the file is read. I have removed those ints in hope that gcc in that case won't add any=20 padding bytes. It works for me. But now I am really surprised why it=20 doesn't work for you. Does gcc add padding bytes anyway? Are there=20 also some padding issues in the TGA format? Can you send me those=20 images you were testing the loader on? Cheers, --=20 Piotr Paw=B3ow mailto:pp...@si... |
From: Piotr <pp...@si...> - 2002-11-06 15:04:13
|
> I have removed those ints in hope that gcc in that case won't add > any padding bytes. It works for me. But now I am really surprised Oh, stupid me, I know what is wrong, datasize calculation. I shouldn't=20 touch the struct. Try this one: --- loader_tga.c Wed Nov 6 15:52:44 2002 +++ loader_tga.new.c Wed Nov 6 15:56:38 2002 @@ -48,6 +48,7 @@ #define TGA_DESC_VERTICAL 0x20 #define TGA_SIGNATURE "TRUEVISION-XFILE" +#define TGA_FOOTER_LEN 26 typedef struct { unsigned char idLength; @@ -223,8 +224,8 @@ return 0; /* read the footer first */ - fseek (fp, 0L - (sizeof (tga_footer)), SEEK_END); - if (fread (&footer, sizeof (tga_footer), 1, fp) !=3D 1) + fseek (fp, 0L - TGA_FOOTER_LEN, SEEK_END); + if (fread (&footer, TGA_FOOTER_LEN, 1, fp) !=3D 1) { fclose(fp); return 0; @@ -333,7 +334,7 @@ stat(im->real_file, &ss); datasize =3D ss.st_size - sizeof(tga_header) - header.idLength - - (footer_present ? sizeof(tga_footer) : 0); + (footer_present ? TGA_FOOTER_LEN : 0); buf =3D malloc(datasize); if(!buf) Thanks. --=20 Piotr Paw=B3ow mailto:pp...@si... |
From: Michael J. <e-...@ka...> - 2003-04-03 19:11:34
|
On Wednesday, 06 November 2002, at 16:04:03 (+0100), Piotr Paw?ow wrote: > > I have removed those ints in hope that gcc in that case won't add > > any padding bytes. It works for me. But now I am really surprised > > Oh, stupid me, I know what is wrong, datasize calculation. I > shouldn't touch the struct. Try this one: The TGA loader works fine as is. This patch, like your last patch, breaks it. Michael -- Michael Jennings (a.k.a. KainX) http://www.kainx.org/ <me...@ka...> n + 1, Inc., http://www.nplus1.net/ Author, Eterm (www.eterm.org) ----------------------------------------------------------------------- "Shouldn't we help her find her way back to a place called home?" "Honey, she's wearing synthetic plaid. It's a four day drive and a boat trip to a place called home." -- Sean Hayes (Jack) and Megan Mullally (Karen), "Will and Grace" |
From: Piotr <pp...@si...> - 2003-04-20 19:20:16
|
On Thursday 03 of April 2003 21:11, Michael Jennings wrote: > The TGA loader works fine as is. This patch, like your last patch, > breaks it. [Head banging on the wall] How could I be soooo stupid!? I somehow assumed that padding bytes are added at the end of the struct only. So instead of fixing the bug, I only changed the case in which it manifests itself :( I should listen to the wiser men on pld-devel telling me to use __attribute__((packed)) instead, like this: ------------------------------------------------------------------- diff -urN imlib2-1.0.6.orig/loaders/loader_tga.c imlib2-1.0.6/loaders/loader_tga.c --- imlib2-1.0.6.orig/loaders/loader_tga.c Thu Oct 25 00:50:02 2001 +++ imlib2-1.0.6/loaders/loader_tga.c Thu Feb 27 00:52:14 2003 @@ -70,7 +70,7 @@ char signature[16]; char dot; char null; -} tga_footer; +} __attribute__((packed)) tga_footer; /* ------------------------------------------------------------------- Sorry for wasting your time, I will go hide under my rock now... <8| -- Signature broken |
From: Kim W. <ki...@wo...> - 2003-04-21 07:53:29
|
Piotr Paw?ow wrote: > On Thursday 03 of April 2003 21:11, Michael Jennings wrote: >=20 >>The TGA loader works fine as is. This patch, like your last patch, >>breaks it. >=20 >=20 > [Head banging on the wall] How could I be soooo stupid!? >=20 > I somehow assumed that padding bytes are added at the end of the struct > only. So instead of fixing the bug, I only changed the case in which it > manifests itself :( >=20 > I should listen to the wiser men on pld-devel telling me to use > __attribute__((packed)) instead, like this: >=20 > ------------------------------------------------------------------- > diff -urN imlib2-1.0.6.orig/loaders/loader_tga.c imlib2-1.0.6/loaders/l= oader_tga.c > --- imlib2-1.0.6.orig/loaders/loader_tga.c Thu Oct 25 00:50:02 200= 1 > +++ imlib2-1.0.6/loaders/loader_tga.c Thu Feb 27 00:52:14 2003 > @@ -70,7 +70,7 @@ > char signature[16]; > char dot; > char null; > -} tga_footer; > +} __attribute__((packed)) tga_footer; >=20 >=20 > /* > ------------------------------------------------------------------- >=20 > Sorry for wasting your time, I will go hide under my rock now... <8| >=20 This only works with gcc. Other compilers will be very unhappy with it. Forced struct packing is very messy across compiler versions, and should=20 instead be worked around in portable code. From loader_tga.c: typedef struct { unsigned int extensionAreaOffset; unsigned int developerDirectoryOffset; char signature[16]; char dot; char null; } tga_footer; This will not match the spec if an int is not 32 bit or if the compiler=20 pads the struct in any way. Recent gcc versions will by default pad this struct to sizeof(int), i.e.=20 4 byte on a standard PC (=3D> trouble). In portable code you would have to do something like: #include <stdint.h> typedef struct { uint32_t extensionAreaOffset; uint32_t developerDirectoryOffset; char signature[16]; char dot; char null; } tga_footer; #define TGA_FOOTER_SIZE 26 /* No less no more */ Use TGA_FOOTER_SIZE in stead of sizeof(tga_footer) throughout the code. I'm not aware to which extent stdint.h exists in different=20 compiler/library versions. It may be necessary with some HAVE_...=20 autoconf magic to cover all targets. /Kim |