Re: [PATCH v2][RESUBMIT] apt-pkg:contrib Avoid compiler warning about mktemp using mkostemp

2013/8/22 David Kalnischkies <kalnischkies+debian@gmail.com>

Hi,

(Thanks for working on this, the warning bugs me for a few days now!)

Thanks for the welcome!

> 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>

Fully agree with this. I just read in somewhere that it was removed, but don't the exact date. When I sent the patch, I look for the exact posix date in online manpage like http://linux.die.net/man/1/mktemp
but it doesn't mention the conforming POSIX. When I read your lines, I found others pages online like
http://www.unix.com/man-page/Linux/3/mktemp/ that include the reference :) Thanks for clarify this.

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.

I think that my last attachment solves that issue. But I see that I forget save the file and include that part. Sorry, I reattach with this changed.

(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).

My first suggestion in IRC with Julian Andres was use mkstemp, but he changes my mind when he suggested to use mkostemp.

17:44 juliank said: shakaran: Good idea, but we need to pass the flags when creating the file; as in the open() call below (and this must not be called anymore). mkostemp might work; but I don't think that our use of mktemp is dangerous.

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.

The problem is that mkstemp opens the file descriptorfor reading and writing flags only (see http://pubs.opengroup.org/onlinepubs/009695399/functions/mkstemp.html ). And we are opening with "fileflags" custom variable depending of atomic or exclusive, or others modes.

I can submit again the patch with this change modified too, so we can use a really POSIX compatible function, but I need clarification for the issue flags for use finally mkostemp or mkstemp. I don't know if we can open with custom fileflags using only mkstemp.

[0] They might not all run correctly currently. They do for me, but it seems
like others see some of them failing sometimes.

I try to run the test, but I don't find documentation in README or a TESTING file or even other places.

After compile all, I try to run:

./test/integration/run-tests

But it complains all with:

E: You need to build tree first

I check the that build folder is compiled, so I don't understand the error. How do you run this test?