On Wed, Nov 11, 2009 at 3:31 PM, Myles Watson <myle...@gmail.com> wrote: > On Wed, Nov 11, 2009 at 4:06 PM, Peter Stuge <pe...@stuge.se> wrote: >> Myles Watson wrote: >>> How about this: >>> >>> Index: src/arch/i386/boot/coreboot_table.c >>> =================================================================== >>> --- src/arch/i386/boot/coreboot_table.c (revision 4931) >>> +++ src/arch/i386/boot/coreboot_table.c (working copy) >>> @@ -485,11 +485,10 @@ >>> >>> #if (CONFIG_HAVE_OPTION_TABLE == 1) >>> { >>> - struct lb_record *rec_dest, *rec_src; >>> - /* Write the option config table... */ >>> + struct lb_record *rec_dest; >>> + /* Copy the option config table, it's already a lb_record... >>> */ >>> rec_dest = lb_new_record(head); >>> - rec_src = (struct lb_record *)(void *)&option_table; >>> - memcpy(rec_dest, rec_src, rec_src->size); >>> + memcpy(rec_dest, &option_table, sizeof(option_table)); >> >> It is completely unclear to me why it is safe to write beyond the >> struct lb_record > lb_record is just the header. The data follows it, but isn't a member > of the struct. > >> (maybe it is an elaborate side-effect of the call to >> lb_new_record()?) > I think lb_new_record uses the size to find the next header location. > Is that what you meant? > >> Acked-by: Peter Stuge <pe...@stuge.se> > Rev 4935.
I am almost certain that this change: >>> - memcpy(rec_dest, rec_src, rec_src->size); >>> + memcpy(rec_dest, &option_table, sizeof(option_table)); completely changes the behavior of the code and is wrong. I'm willing to be convinced. But sizeof(option_table) is 8 and rec_src->size is 1160. So you were copying the whole record and now you're copying a record header. I don't see how this can be a good idea. Am I missing something? It seems to me to change a compiler warning you may have broken what the code is supposed to do. ron -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot