Re: [PATCH v2][RESUBMIT] apt-pkg:contrib Avoid compiler warning about mktemp using mkostemp
Hi,
(Thanks for working on this, the warning bugs me for a few days now!)
> 2013/8/21 Julian Andres Klode <jak@debian.org>
>> On Wed, Aug 21, 2013 at 08:51:54PM +0200, Angel Guzman Maeso wrote:
>> > better use `mkstemp' or `mkdtemp'"· mktemp is not standard POSIX
>> > function. This change it is only for avoid the warning, but it is
>> > not scrictly neccesary to apply because mktemp don't do anything
>> > unsafe in this piece of code.
>>
>> "This change it is only for avoid" is not valid English, just like
>> the "don't" in this case => "doesn't" or "does not".
<nitpick>
The reasoning is a bit wrong. mktemp is removed in POSIX.1-2008, so it was
a standard function until "recently". Thats a difference to something like
mkostemp which is really non-POSIX.
Also this FileFd feature as far as APT uses it might not be a security
problem, but it might be for other users of the library.
</nitpick>
As Julian already said you need to move the TemporaryFileName assignment
below the mkostemp as otherwise TemporaryFileName contains the template
rather than the "real" name, so the replacement later on does not work.
(You might want to have a look into apt/test/integration/ as it contains
a bunch of testscripts[0] you can run to test your patch(es) a bit)
The other thing is: You are using mkostemp, which is a non-POSIX glibc
extension. Looking over the usage we have, we should get away with "just"
mkstemp which is POSIX (not that we couldn't use non-POSIX functions, but
if we can, I see no reason not to try to avoid them).
The reason is as follows: Atomic means the file should be opened exclusively,
which mkstemp (and all others) do by default. Exclusive is stronger than
Create (O_CREAT) or Empty (O_TRUNC) which are the other modes we provide and
are trivially statisfied for the temporary file.
I don't see us providing other modes in FileFd in the near future and even
if we would do, we could still just flip over to the extensions if the need
arises.
(Also, I wonder a bit if it is okay to call FileFdErrno() after free() as it
could potentially reset errno – even though free doesn't use errno as far
as it looks, so that is probably okay)
Best regards
David Kalnischkies
[0] They might not all run correctly currently. They do for me, but it seems
like others see some of them failing sometimes.
Reply to: