|
From: Dallman, J. <joh...@si...> - 2008-04-23 09:51:55
|
I'm learning to use Valgrind, with version 3.3.0 running on AMD64
SLES10sp1 with the GCC 4.1.2 that comes with that SLES.
I have this small structure:
typedef struct GRA_colour_s
{
unsigned short red;
unsigned short green;
unsigned short blue;
} GRA_colour_t;
It forms part of a larger structure GRA_tag_t, which totals 72 bytes
on AMD64, with a GRA_colour_t as the last element, at offset 66.
Valgrind complains:
==14617== Invalid read of size 8
==14617== at 0x438714: GRA__item_draw (gra_geometry.c:2911)
==14617== by 0x43A646: GRA_item_draw (gra_geometry.c:3231)
...
==14617== Address 0x5c29f7a is 66 bytes inside a block of size 72
alloc'd
==14617== at 0x4A1FDEB: malloc (vg_replace_malloc.c:207)
==14617== by 0x43A7FD: GRA__mem_alloc (gra_mem.c:110)
That block of 72 bytes is a malloc'ed GRA_tag_t, of course.
The code at the place with the read calls a function, passing the
GRA_colour_t from the GRA_tag_t, by value, as the only argument to
the function call. Looking at the machine code in GDB, GCC is
using a single 64-bit load to pick up the entire GRA_colour_t,
along with the two following bytes, and put it in a register for
passing to the function. It does this even at -O0.
The only obvious thing to do to get rid of the Valgrind complaint
seems to be to change GRA_colour_t to:
typedef struct GRA_colour_s
{
unsigned short red;
unsigned short green;
unsigned short blue;
unsigned short spare;
} GRA_colour_t;
But that's not very nice. Any other suggestions?
thanks,
--
John Dallman
Parasolid Porting Engineer
|
|
From: Julian S. <js...@ac...> - 2008-04-23 10:05:09
|
> typedef struct GRA_colour_s
> {
> unsigned short red;
> unsigned short green;
> unsigned short blue;
> } GRA_colour_t;
> [...]
> the function call. Looking at the machine code in GDB, GCC is
> using a single 64-bit load to pick up the entire GRA_colour_t,
> along with the two following bytes, and put it in a register for
> passing to the function. It does this even at -O0.
Sigh. Then gcc is generating code which is in violation of ISO
in that it reads beyond the end of an allocated block.
You can probably make V shut up by giving it --partial-loads-ok=yes,
although that's also liable to hide real bugs in some circumstances.
But there's something else troubling about this, assuming your
analysis is right. Suppose that you put a GRA_colour_t structure
6 bytes before a page boundary, and mprotect the next page so it's
not readable.
You can then safely fill in the red/green/blue fields individually
with 2-byte stores.
But if you then pass the structure by value, gcc generates an 8 byte
load, which includes 2 bytes in the protected page, and the program
segfaults. Even though no part of the structure is on the protected
page. Right?
J
|
|
From: Dallman, J. <joh...@si...> - 2008-04-23 10:59:51
|
Julian Seward [mailto:js...@ac...] wrote: > Sigh. Then gcc is generating code which is in violation of ISO > in that it reads beyond the end of an allocated block. ... > But there's something else troubling about this, assuming your > analysis is right. Suppose that you put a GRA_colour_t structure > 6 bytes before a page boundary, and mprotect the next page so it's > not readable. > > You can then safely fill in the red/green/blue fields individually > with 2-byte stores. > > But if you then pass the structure by value, gcc generates an 8 byte > load, which includes 2 bytes in the protected page, and the program > segfaults. Even though no part of the structure is on the protected > page. Right? Yup. I guess I'd better get a repro case together for a GCC bug report, which may result in my proving myself wrong. I'll tolerate the noise from valgrind for now; there isn't very much of it. -- John Dallman Parasolid Porting Engineer |
|
From: Dallman, J. <joh...@si...> - 2008-04-24 15:18:23
|
Julian Seward [mailto:js...@ac...] write:
> Sigh. Then gcc is generating code which is in violation of ISO
> in that it reads beyond the end of an allocated block.
Sure seems to be. Here's the simple example:
/* ===============================================================
gcc_struct_bug.c
Demonstration program for a problem with GCC and malloc'ed
structs on 64-bit x86 Linux, demonstrated with Valgrind 3.3.0's
Memcheck.
The example Linux was 64-bit SuSE Linux Enterprise Server 10
with Service Pack 1, and the GCC supplied with it: "gcc (GCC)
4.1.2 20070115 (prerelease) (SUSE Linux)".
Build with "gcc -m64 -g -O0 gcc_struct_bug.c -o gcc_struct_bug."
Verify that it runs and then run it through Valgrind. The error
that comes out is:
==25937== Invalid read of size 8
==25937== at 0x40069F: main (gcc_struct_bug.c:74)
==25937== Address 0x4d55030 is 0 bytes inside a block of size 6 alloc'd
==25937== at 0x4A1FDEB: malloc (vg_replace_malloc.c:207)
==25937== by 0x400672: main (gcc_struct_bug.c:69)
That happens because at point A, marked below, GCC generates a
single 8-byte load to get the structure at *c into a register
for passing to set_colour(). Since the malloc'ed block is only
six bytes, this involves reading off the end. The GDB disassembly
of that line, on my example machine, reads:
0x000000000040069b <main+65>: mov 0xfffffffffffffff8(%rbp),%rax
0x000000000040069f <main+69>: mov (%rax),%rdi
0x00000000004006a2 <main+72>: callq 0x400628 <set_colour>
This breaks ISO C rules in that it reads off the end of a
malloc'ed block. It is not harmless: were the structure six
bytes before a page boundary - easily achieved if it were an
element of a larger structure - and the following page
mprotect'ed, the read would cause a segmentation violation.
The problem is suppressed in this simple example at -O3,
seemingly through the variables being optimised out of
existence, but that isn't applicable to the far more complex
code within which the problem was discovered, which doesn't work
at -O3.
Using GCC's -fmudflap -lmudflap options suppresses the valgrind
error, but causes a great deal more memory allocation and
freeing.
=============================================================== */
#include <stdio.h>
#include <stdlib.h>
typedef struct colour_s
{
unsigned short red;
unsigned short green;
unsigned short blue;
} colour_t;
void set_colour( colour_t col)
{
printf( "colour is %d %d %d\n",
col.red, col.green, col.blue);
}
int main( int argc, char *argv[])
{
colour_t *c;
if( (c = (colour_t*)malloc( sizeof(colour_t))) != NULL)
{
c->red = 115;
c->green = 122;
c->blue = 98;
set_colour( *c); /* Point A - GCC uses an eight-byte
load to load a six-byte structure
into a register for passing, which
involves reading off the end of the
malloc'ed block. */
free( c); /* Free the memory */
exit( 0);
}
else
{
printf( "Failed to allocate a colour_t\n");
exit(1);
}
}
--
John Dallman
Parasolid Porting Engineer
|
|
From: Christoph B. <bar...@or...> - 2008-04-24 16:20:15
|
Am Donnerstag, 24. April 2008 schrieb Dallman, John:
> This breaks ISO C rules in that it reads off the end of a
> malloc'ed block. It is not harmless: were the structure six
> bytes before a page boundary - easily achieved if it were an
> element of a larger structure - and the following page
> mprotect'ed, the read would cause a segmentation violation.
One can easily change your example into a program that crashes in the same
line and it does not help to use -O3:
#include <sys/mman.h>
#include <stdio.h>
#include <stdlib.h>
typedef struct colour_s
{
unsigned short red;
unsigned short green;
unsigned short blue;
} colour_t;
void set_colour( colour_t col)
{
printf( "colour is %d %d %d\n",
col.red, col.green, col.blue);
}
int main( int argc, char *argv[])
{
colour_t * c = mmap(NULL, sizeof(colour_t) * 2048, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
if (c != MAP_FAILED)
{
c[2047].red = 115;
c[2047].green = 122;
c[2047].blue = 98;
set_colour( c[2047]); /* Point A - GCC uses an eight-byte
load to load a six-byte structure
into a register for passing, which
involves reading off the end of the
malloc'ed block. */
munmap(c,sizeof(colour_t) * 2048);
exit( 0);
}
else
{
printf( "Failed to allocate a colour_t\n");
exit(1);
}
}
|
|
From: Christoph B. <bar...@or...> - 2008-04-24 16:20:42
|
Am Donnerstag, 24. April 2008 schrieb Dallman, John: > Julian Seward [mailto:js...@ac...] write: > > Sigh. Then gcc is generating code which is in violation of ISO > > in that it reads beyond the end of an allocated block. > > Sure seems to be. Here's the simple example: Did you already file a bug report for GCC? |
|
From: Dallman, J. <joh...@si...> - 2008-04-24 17:56:04
|
> Did you already file a bug report for GCC? No, because I've never done that and don't know how. Any pointers will be welcome. -- John Dallman Parasolid Porting Engineer |
|
From: Christoph B. <bar...@or...> - 2008-04-24 18:14:03
|
Am Donnerstag, 24. April 2008 schrieb Dallman, John: > > Did you already file a bug report for GCC? > > No, because I've never done that and don't know how. Any > pointers will be welcome. Go to gcc.gnu.org and then to the bug database. Login and create a new bug. If you want I can file it for you. Christoph |
|
From: Julian S. <js...@ac...> - 2008-04-24 21:57:51
|
On Thursday 24 April 2008 20:14, Christoph Bartoschek wrote: > Am Donnerstag, 24. April 2008 schrieb Dallman, John: > > > Did you already file a bug report for GCC? > > > > No, because I've never done that and don't know how. Any > > pointers will be welcome. > > Go to gcc.gnu.org and then to the bug database. Login and create a new bug. > > If you want I can file it for you. Let me ask first, does anyone know of any reason why the test program (which segfaults) could be declared to be invalid C, and so gcc's code could then be said to be valid? Also, it's worth checking the problem is reproducible on the latest stable gcc (4.3.0). Filing bug reports on older gccs is imo pretty much pointless. J |
|
From: Dallman, J. <joh...@si...> - 2008-04-25 14:21:16
|
Andi Kleen [mailto:an...@fi...] wrote: > Julian Seward <js...@ac...> writes: > > Also, it's worth checking the problem is reproducible on the > > latest stable gcc (4.3.0). Filing bug reports on older gccs is > > imo pretty much pointless. > When he has a support contract for his SLES he could file a bug > report with Novell. That might result in a fix for that particular > gcc version. That's pretty much the plan, I've referred them to the GCC bugzilla bug. thanks, -- John Dallman Parasolid Porting Engineer |
|
From: Christoph B. <bar...@or...> - 2008-04-24 22:08:01
|
Am Donnerstag, 24. April 2008 schrieb Julian Seward: > Let me ask first, does anyone know of any reason why the test > program (which segfaults) could be declared to be invalid C, > and so gcc's code could then be said to be valid? > > Also, it's worth checking the problem is reproducible on the > latest stable gcc (4.3.0). Filing bug reports on older gccs is > imo pretty much pointless. I've tested it with 4.3.0 and it segfaults. The program is not pure C because it uses linux specific things, but it should be valid. Christoph |
|
From: Julian S. <js...@ac...> - 2008-04-24 22:23:02
|
On Friday 25 April 2008 00:08, Christoph Bartoschek wrote: > Am Donnerstag, 24. April 2008 schrieb Julian Seward: > > Let me ask first, does anyone know of any reason why the test > > program (which segfaults) could be declared to be invalid C, > > and so gcc's code could then be said to be valid? > > > > Also, it's worth checking the problem is reproducible on the > > latest stable gcc (4.3.0). Filing bug reports on older gccs is > > imo pretty much pointless. > > I've tested it with 4.3.0 and it segfaults. The program is not pure C > because it uses linux specific things, but it should be valid. Ok, good. The real reason for my question (is it valid C?) is that the GCC folks have amongst them a good collection of language lawyers. They are very good at thinking up reasons why programs like the test case are somehow not exactly compliant with the C standards, and so it is ok for gcc to produce apparently-broken code as we see. So I'm just trying to get the test case as well sanity-checked as we can, before sending it off to gcc-land. J |
|
From: Julian S. <js...@ac...> - 2008-04-24 22:39:38
|
Christoph, > So I'm just trying to get the test case as well sanity-checked as we > can, before sending it off to gcc-land. Having looked at your test, it does look OK to me. Can one or the other of you file a gcc bug report? J |
|
From: Dallman, J. <joh...@si...> - 2008-04-25 08:43:03
|
Julian Seward [mailto:js...@ac...] wrote: > Having looked at your test, it does look OK to me. > > Can one or the other of you file a gcc bug report? Christoph, you wrote the one that demonstrates a nice segfault without needing Valgrind to demonstrate the problem, so I guess the honour is yours, unless you'd prefer me to do it. Please tell me the bug number so that I can chase SuSE to get a fix into their next service pack. (I'm constrained from just installing updated versions of GCC because the product I work on is a large and complex library, and customers get very nervous if I'm using a different compiler from them; so much so that they start reporting nonsense bugs). -- John Dallman Parasolid Porting Engineer |
|
From: Christoph B. <bar...@or...> - 2008-04-25 09:30:43
|
Am Freitag, 25. April 2008 schrieb Dallman, John: > Christoph, you wrote the one that demonstrates a nice segfault > without needing Valgrind to demonstrate the problem, so I guess > the honour is yours, unless you'd prefer me to do it. Please > tell me the bug number so that I can chase SuSE to get a fix > into their next service pack. You can add yourself to the cc list: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36043 |
|
From: Dallman, J. <joh...@si...> - 2008-04-25 09:58:56
|
Christoph Bartoschek [mailto:bar...@or...] wrote: > You can add yourself to the cc list: > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36043 Done. Thanks very much. -- John Dallman Parasolid Porting Engineer |