Re: [Patch mach-o/bfd] improve build command routine robustness.

[Tristan Gingold <gingold at adacore dot com>]

On Jan 9, 2012, at 12:01 PM, Iain Sandoe wrote:

> Although I do not see the reported build warning with 4.6 or 4.7 as the host compiler, I can see where it could arise.
> 
> Additionally, when looking harder at this, it seems that almost any permutation of commands is valid - the following patch improves robustness - and should fix the reported warning.

Hi,

thank you for working on that.

See comments inside.

(I have committed an obvious patch to fix the reported warning).

Tristan.

> OK?
> Iain
> 
> bfd:
> 
> 	* mach-o.c (bfd_mach_o_build_commands): Make the building of each command
> 	type independent.
> 
> bfd/mach-o.c |  129 +++++++++++++++++++++++++++++++++++----------------------
> 1 files changed, 79 insertions(+), 50 deletions(-)
> 
> diff --git a/bfd/mach-o.c b/bfd/mach-o.c
> index 07ca65a..a1f6596 100644
> --- a/bfd/mach-o.c
> +++ b/bfd/mach-o.c
> @@ -2149,15 +2149,19 @@ bfd_mach_o_build_dysymtab_command (bfd *abfd,
>    TODO: Other file formats, rebuilding symtab/dysymtab commands for strip
>    and copy functionality.  */
> 
> +#define MACHO_SEG_CMD_PRESENT	0x0001
> +#define MACHO_SYM_CMD_PRESENT	0x0002
> +#define MACHO_DYSYM_CMD_PRESENT	0x0004
> +
> bfd_boolean
> bfd_mach_o_build_commands (bfd *abfd)
> {
>   bfd_mach_o_data_struct *mdata = bfd_mach_o_get_data (abfd);
> -  unsigned int wide = mach_o_wide_p (&mdata->header);
> -  bfd_mach_o_segment_command *seg;
> -  bfd_mach_o_load_command *cmd;
> -  bfd_mach_o_load_command *symtab_cmd;
> -  unsigned symcind;
> +  unsigned wide = mach_o_wide_p (&mdata->header);
> +  unsigned present = 0;
> +  unsigned symind = 0;
> +  unsigned dysymind = 0;

I think you should get rid of 'present', make symind, dysymind signed, and use -1 for not present.  This is easier to read (IMHO).
BTW, feel free to rename these variables (e.g.: symind -> symtab_idx, dysymind -> dysymtab_idx).

> +  unsigned long base_offset = 0;
> 
>   /* Return now if commands are already present.  */
>   if (mdata->header.ncmds)
> @@ -2187,31 +2191,49 @@ bfd_mach_o_build_commands (bfd *abfd)
>   if (!bfd_mach_o_mangle_symbols (abfd))
>     return FALSE;
> 
> -  /* It's valid to have a file with only absolute symbols...  */
> +  /* Very simple command set (only really applicable to MH_OBJECTs):
> +     All the commands are optional - present only when there is suitable data.
> +     (i.e. it is valid to have an empty file)
> +
> +	a command (segment) to contain all the sections,
> +	command for the symbol table,
> +	a command for the dysymtab.
> +
> +     ??? maybe we should assert that this is an MH_OBJECT?  */
> +
>   if (mdata->nsects > 0)
>     {
>       mdata->header.ncmds = 1;
> -      symcind = 1;
> +      symind = 1;
> +      dysymind = 1;
> +      present |= MACHO_SEG_CMD_PRESENT;
>     }
> -  else
> -    symcind = 0;
> 
> -  /* It's OK to have a file with only section statements.  */
>   if (bfd_get_symcount (abfd) > 0)
> -    mdata->header.ncmds += 1;
> +    {
> +      mdata->header.ncmds += 1;

The usual C idiom is '++' (likewise below).

> +      dysymind += 1;
> +      present |= MACHO_SYM_CMD_PRESENT;
> +    }
> 
> -  /* Very simple version (only really applicable to MH_OBJECTs):
> -	a command (segment) to contain all the sections,
> -	a command for the symbol table
> -	a n (optional) command for the dysymtab.
> +  /* FIXME:
> +     This is a rather crude test for whether we should build a dysymtab.  */
> +  if (bfd_mach_o_should_emit_dysymtab ()
> +      && bfd_get_symcount (abfd))
> +    {
> +      mdata->header.ncmds += 1;
> +      present |= MACHO_DYSYM_CMD_PRESENT;
> +    }
> 
> -     ??? maybe we should assert that this is an MH_OBJECT?  */
> +  if (wide)
> +    base_offset = BFD_MACH_O_HEADER_64_SIZE;
> +  else
> +    base_offset = BFD_MACH_O_HEADER_SIZE;
> 
> -  if (bfd_mach_o_should_emit_dysymtab ()
> -      && bfd_get_symcount (abfd) > 0)
> -    mdata->header.ncmds += 1;
> +  /* Well, we must have a header, at least.  */
> +  mdata->filelen = base_offset;
> 
> -  /* A bit weird, but looks like no content;
> +  /* A bit unusual, but no content is valid;
>      as -n empty.s -o empty.o  */
>   if (mdata->header.ncmds == 0)
>     return TRUE;
> @@ -2221,10 +2243,10 @@ bfd_mach_o_build_commands (bfd *abfd)
>   if (mdata->commands == NULL)
>     return FALSE;
> 
> -  if (mdata->nsects > 0)
> +  if (present & MACHO_SEG_CMD_PRESENT)
>     {
> -      cmd = &mdata->commands[0];
> -      seg = &cmd->command.segment;
> +      bfd_mach_o_load_command *cmd = &mdata->commands[0];
> +      bfd_mach_o_segment_command *seg = &cmd->command.segment;
> 
>       /* Count the segctions in the special blank segment used for MH_OBJECT.  */
>       seg->nsects = bfd_mach_o_count_sections_for_seg (NULL, mdata);
> @@ -2232,50 +2254,57 @@ bfd_mach_o_build_commands (bfd *abfd)
> 	return FALSE;
> 
>       /* Init segment command.  */
> +      cmd->offset = base_offset;
>       if (wide)
> 	{
> 	  cmd->type = BFD_MACH_O_LC_SEGMENT_64;
> -	  cmd->offset = BFD_MACH_O_HEADER_64_SIZE;
> 	  cmd->len = BFD_MACH_O_LC_SEGMENT_64_SIZE
> 			+ BFD_MACH_O_SECTION_64_SIZE * seg->nsects;
> 	}
>       else
> 	{
> 	  cmd->type = BFD_MACH_O_LC_SEGMENT;
> -	  cmd->offset = BFD_MACH_O_HEADER_SIZE;
> 	  cmd->len = BFD_MACH_O_LC_SEGMENT_SIZE
> 			+ BFD_MACH_O_SECTION_SIZE * seg->nsects;
> 	}
> +
>       cmd->type_required = FALSE;
>       mdata->header.sizeofcmds = cmd->len;
> -      mdata->filelen = cmd->offset + cmd->len;
> +      mdata->filelen += cmd->len;
>     }
> 
> -  if (bfd_get_symcount (abfd) > 0)
> +  if (present & MACHO_SYM_CMD_PRESENT)
>     {
>       /* Init symtab command.  */
> -      symtab_cmd = &mdata->commands[symcind];
> -
> -      symtab_cmd->type = BFD_MACH_O_LC_SYMTAB;
> -      if (symcind > 0)
> -        symtab_cmd->offset = mdata->commands[0].offset
> -			     + mdata->commands[0].len;
> -      else
> -        symtab_cmd->offset = 0;
> -      symtab_cmd->len = 6 * 4;
> -      symtab_cmd->type_required = FALSE;
> +      bfd_mach_o_load_command *cmd = &mdata->commands[symind];
> 
> -      mdata->header.sizeofcmds += symtab_cmd->len;
> -      mdata->filelen += symtab_cmd->len;
> +      cmd->type = BFD_MACH_O_LC_SYMTAB;
> +      cmd->offset = base_offset;
> +      if (present & MACHO_SEG_CMD_PRESENT)
> +        cmd->offset += mdata->commands[0].len;
> +
> +      cmd->len = 6 * 4;

Please, use:
sizeof (struct mach_o_symtab_command_external) + sizeof (struct mach_o_load_command_external)
or the ..._SIZE constants.

> +      cmd->type_required = FALSE;
> +      mdata->header.sizeofcmds += cmd->len;
> +      mdata->filelen += cmd->len;
>     }
> 
> -  /* If required, setup symtab command.  */
> -  if (bfd_mach_o_should_emit_dysymtab ()
> -      && bfd_get_symcount (abfd) > 0)
> +  /* If required, setup symtab command, see comment above about the quality
> +     of this test.  */
> +  if (present & MACHO_DYSYM_CMD_PRESENT)
>     {
> -      cmd = &mdata->commands[symcind+1];
> +      bfd_mach_o_load_command *cmd = &mdata->commands[dysymind];
> +
>       cmd->type = BFD_MACH_O_LC_DYSYMTAB;
> -      cmd->offset = symtab_cmd->offset + symtab_cmd->len;
> +      if (present & MACHO_SYM_CMD_PRESENT)
> +        cmd->offset = mdata->commands[symind].offset
> +		    + mdata->commands[symind].len;
> +      else if (present & MACHO_SEG_CMD_PRESENT)
> +        cmd->offset = mdata->commands[0].offset
> +		    + mdata->commands[0].len;
> +      else
> +	cmd->offset = base_offset;
> +
>       cmd->type_required = FALSE;
>       cmd->len = 18 * 4 + BFD_MACH_O_LC_SIZE;

Likewise.

> 
> @@ -2285,18 +2314,18 @@ bfd_mach_o_build_commands (bfd *abfd)
> 
>   /* So, now we have sized the commands and the filelen set to that.
>      Now we can build the segment command and set the section file offsets.  */
> -  if (mdata->nsects > 0
> -      && ! bfd_mach_o_build_seg_command (NULL, mdata, seg))
> +  if ((present & MACHO_SEG_CMD_PRESENT)
> +      && ! bfd_mach_o_build_seg_command (NULL, mdata,
> +					 &mdata->commands[0].command.segment))
>     return FALSE;
> 
>   /* If we're doing a dysymtab, cmd points to its load command.  */
> -  if (bfd_mach_o_should_emit_dysymtab ()
> -      && bfd_get_symcount (abfd) > 0
> +  if ((present & MACHO_DYSYM_CMD_PRESENT)
>       && ! bfd_mach_o_build_dysymtab_command (abfd, mdata,
> -					      &mdata->commands[symcind+1]))
> +					      &mdata->commands[dysymind]))
>     return FALSE;
> 
> -  /* The symtab command is filled in when the symtab is written.  */
> +  /* The symtab command is filled when the symtab is written.  */
>   return TRUE;
> }

Good idea to harden this!

Tristan.