[PATCH v1 1/1] ldconfig: add --install option

[Collin Funk]

Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> writes:

>> +      if (argv[remaining] == NULL)
>> +	error (EXIT_FAILURE, 0, _("Missing source file name"));
>> +
>> +      char *source = (opt_chroot
>> +		      ? chroot_canon (opt_chroot, argv[remaining])
>> +		      : argv[remaining]);
>> +      if (source == NULL)
>> +	error (EXIT_FAILURE, errno, _("Can't find %s"), argv[remaining]);
>> +
>> +      int src_fd = open (source, O_RDONLY);
>> +      if (src_fd < 0)
>> +	error (EXIT_FAILURE, errno, _("Can't open %s"), source);
>> +
>> +      char *dest = xmalloc (strlen (cache_file) + 6 + 1);
>> +
>> +      sprintf(dest, "%sXXXXXX", cache_file);
>> +      int dest_fd = mkstemp (dest);
>> +      if (dest_fd < 0)
>> +	error (EXIT_FAILURE, errno, _("Can't create %s"), dest);
>
> This will leak the temporary file if an error happens. save_cache in 
> elf/cache.c:677-736 uses a fixed cache_name~ suffix, so it is implicitly 
> overwritten on next run.
>
> Maybe it would be better to consolidate this temporary file creation and 
> use only one strategy, with possible adding also an atexit hook to delete
> temporary files.

Using atexit would still have a file left over if the program exits
abnormally, no? I think you would need a signal handler that cleans it
up.

>> +
>> +      struct stat st;
>> +      if (fstat (src_fd, &st) < 0)
>> +	error (EXIT_FAILURE, errno, _("Can't stat %s"), source);
>> +
>> +      if (copy_file_range (src_fd, NULL, dest_fd, NULL, st.st_size, 0)
>> +	  < st.st_size)
>> +	error (EXIT_FAILURE, errno, _("Can't copy to %s"), dest);
>
> Unfortunately copy_file_range only properly support on Linux, this will always
> fail on Hurd which uses the generic io/copy_file_range.c. Also, copy_file_range
> has many issues before Linux 5.3 [1] (and gnulib wrapper returns ENOSYS in
> this case).

Aside from those issues a working copy_file_range might copy only some
of the requested bytes. You would have to call copy_file_range until it
returns zero to ensure the file is copied in full. With that change you
can also avoid calling fstat and trusting the file size. I doubt that
particularly matters here, though.

> One option would to copy the lib/copy-file.c gnulib module, which handles 
> copy_file failures with a fallback read/write.

It is GPLv3+, so I don't think it could be used here. At least, not
without changing that.

>> +
>> +      /* Make sure user can always read cache file */
>> +      if (chmod (dest, S_IROTH|S_IRGRP|S_IRUSR|S_IWUSR))
>
> Maybe use fchmod since it already has a dest_fp opened.

Agreed.

Collin