On Fri, Aug 23, 2013 at 3:21 AM, Angel Guzman Maeso <shakaran@gmail.com> wrote:
> 2013/8/22 David Kalnischkies <kalnischkies+debian@gmail.com>Hi again! :)
>> (Thanks for working on this, the warning bugs me for a few days now!)
> Thanks for the welcome!
A quick semi-unrelated advice: I will CC you the last time with this mail
unrequested as this is against the usual practice on debian mailinglists.
If you want to be CC'ed you are supposed to request that explicitly –
otherwise it is assumed that you are subscribed to the mailinglist you are
writing to (In other words: No need to CC me or others).
"apt-get install manpages-dev" and you should have all syscalls (2) and
>> > 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.
functions (3) readily available on your system and no need to search for
them online with the added bonus of having the documentation for those
which are actually on your system (plus portability notes).
As I tried to explain already the flags we use can be translated as follows:>> 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.
Create = O_CREAT
Empty = O_TRUNC
Exclusive = O_EXCL
Atomic = O_EXCL (with a temporary file replacing the real file on close)
The source you are quoting is a bit short in describing what mkstemp does,
"man mkstemp" will tell us that mkstemp is opening the tmpfile with O_EXCL,
which is what we need (and is stronger than O_CREAT and O_TRUNC as the
tmpfile is guaranteed to not exist, so a file is always created and never
truncated).
The ReadOnly bit makes no sense in Atomic mode (there isn't that much to be
read from a freshly created empty file) - and is "honored" by FileFd::Open()
with an error message already. That libc will open the temporary file in
ReadWrite mode is not a problem for us, either this was what we needed anyway
or we will write to it only, just that the libc doesn't know about that.
(FileFd needs this information mostly for supporting compressed files as we
can't open a file ReadWrite via a compressor, but that can happen here –
and ReadWrite was the default mode before we supported compressed files…).
The mkstemp manpage mentions a few flags for which it makes sense to use
mkostemp like O_ASYNC, but we do not support setting them, so I don't see a
flag which makes actually sense to set via mkostemp.
So to cut this story short: I have applied your patch (with a slightly
reworded commit message) and commited my own mkostemp → mkstemp over it
to have a proper record of who is to blame if that will ever be a problem.
It also corrects a duplication in your patch: No need to call strerror in
an error message, that is what GlobalError::Errno (via FileFdErrno) will do
for us anyway.
And to end this paragraph properly: Thanks a lot for patch! (and for bearing
with me thoughout all this nitpickery – I am german ;) )
Yeah, my fault. I started writing them to simplify testing my code for GSoC
>> [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.
in 2010, I hadn't thought that they will end up being used for more/by others
hence there is zero documentation or even comments in the code…
I should really change that.
Huh, that is strange. How do you build APT?
> 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?
The message is triggered by a simple file-existence check which should boil
down to something like: test -f build/bin/apt-get
The following worked just fine for me:
git clean -dfx # just to be sure that my checkout is pristine
make
./test/integration/run-tests