[PATCH v7 1/4] Add system-wide tunables: ldconfig part

[DJ Delorie]

Arjun Shankar <arjun@redhat.com> writes:

>> +      /* Check that COUNT won't overflow our data block.  */
>> +      assert (ext->sections[cache_extension_tag_tunables].base
>> +             + ext->sections[cache_extension_tag_tunables].size
>> +             >= (void *) & tec[count]);
>
> Should we make it stricter with a check for exact size? Something like:
>
>      sizeof (tunable_header_cached)
>      + count * sizeof (tunable_entry_cached)
>      == ext->sections[cache_extension_tag_tunables].size
>
> I'm also wondering if it's worth checking for overflow before the
> addition. Maybe it's overkill, but I wanted to mention it.

I changed it to an exact check.  I thought there might be padding but
there isn't.  With an exact check, I don't think we need to worry about
overflow because it won't match.

>> +  struct tunable_header_cached *tunable_data;
>> +  size_t tunable_size;
>> +  size_t tunable_aligner = 0;
>> +
>> +  tunable_data = get_tunconf_ext (str_offset);
>
> This is non-NULL even when there are no tunables, leading to a
> superfluous extension section and data being written here.

Making it not store the tunables section when there are no tunables sent
me down quite a rabbit hole :-)

>> @@ -1164,6 +1179,8 @@ main (int argc, char **argv)
>>
>>    search_dirs ();
>>
>> +  parse_tunconf (tunconfig_file, opt_chroot);
>> +
>
> Looks like it could be guarded by opt_build_cache since nothing else
> consumes the parsed data.

You still want the syntax errors and warnings though.

>> +++ b/elf/tunconf.c
> ...
>> +/* Declared in chroot_canon.c.  */
>> +extern char *chroot_canon (const char *chroot, const char *name);
>
> Looks like it's declared in sysdeps/generic/ldconfig.h.  Not very sure
> if that gets overridden by some non-generic variant.

Fixed.

>> +struct tunable_entry_int {
>> +  struct stringtable_entry *name;
>> +  struct stringtable_entry *value;
>> +  TOP top;
>> +  int tunable_id;
>> +  int value_is_negative:1;
>> +  int value_was_parsed:1;
>> +  unsigned long long value_ull;
>> +  signed long long value_sll;
>
> These last four are never written to by ldconfig.

Added.

>> +struct tunable_entry_int *entry_list;
>> +struct tunable_entry_int **entry_list_next = &entry_list;
>
> This could be local to add_tunable. It doesn't appear to be used elsewhere.

Done.

> In add_tunable:
>
>> +  id = -1;
>> +  for (i = 0; i < array_length (tunable_list); i ++)
>> +    if (strcmp (tunable_list[i].name, name) == 0)
>> +      {
>> +       id = i;
>> +       break;
>> +      }
>
> Here, when we don't find the tunable (id == -1), we store it anyway. This
> makes sense since ldconfig might not know about a valid tunable. Is this
> worth reporting to the user? A message can guard against situations where a
> subtly misspelled or typo-ed tunable doesn't get silently ignored at caching
> time as well as loading time.

I added a warning.

>> +  entry = (struct tunable_entry_int *) xcalloc (sizeof (struct tunable_entry_int), 1);
>> +  entry->name = cache_store_string (name);
>> +  entry->value = cache_store_string (value);
>> +  entry->tunable_id = id;
>> +  entry->top = top;
>
> The parsing of numeric tunable values into the remaining fields should
> happen here, if at all.

Added.

>> +struct tunable_header_cached *
>> +get_tunconf_ext (uint32_t string_table_offset)
>> +{
>> +  struct tunable_entry_int *tei;
>> +  struct tunable_header_cached *thc;
>> +  size_t count;
>> +  size_t size;
>> +
>> +  /* First, count the number of entries we have.  */
>> +  tei = entry_list;
>> +  count = 0;
>> +  while (tei != NULL)
>> +    {
>> +      ++ count;
>> +      tei = tei->next;
>> +    }
>
> Can we return early here with an empty/NULL result when count is 0?

Done!

>> +  /* Allocate enough space for the whole cached block.  */
>> +  size = sizeof (struct tunable_header_cached)
>> +       + sizeof (struct tunable_entry_cached) * count;
>> +  thc = (struct tunable_header_cached *) malloc (size);
>> +
>> +  if (thc == NULL)
>> +    {
>> +      error (0, 0, _("Unable to allocate %zu bytes in get_tunable_ext"), size);
>
> Typo. Should be get_tunconf_ext.

Fixed.

>> +      return NULL;
>> +    }
>> +
>> +  /* Now, fill in the structures.  */
>> +
>> +  thc->signature = TUNCONF_SIGNATURE;
>> +  thc->version = TUNCONF_VERSION;
>> +  thc->num_tunables = count;
>> +  thc->unused_1 = 0;
>> +
>> +  tei = entry_list;
>> +  count = 0;
>> +  while (tei != NULL)
>> +    {
>> +      struct tunable_entry_cached *tec;
>> +
>> +      tec = & ( thc->tunables[count] );
>> +
>> +      tec->flags = 0;
>> +      if (tei->value_was_parsed)
>
> This will always be false.

Not any more :-)