Re: [PATCH v2][RESUBMIT] apt-pkg:contrib Avoid compiler warning about mktemp using mkostemp
On Wed, Aug 21, 2013 at 08:51:54PM +0200, Angel Guzman Maeso wrote:
> Hi apt developers,
>
> apt-pkg:contrib Avoid compiler warning about mktemp using mkostemp
I don't think we need the apt-pkg:contrib part in the description.
>
> The fix avoid the warning "the use of `mktemp' is dangerous,
Drop "the fix". It sounds better then.
> 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".
> Since mkostemp call return a file descriptor the logic for
> TemporaryFileName has been changed for get the same results than
> the original version. The file permission are respected using
> fchmod.
Grammar:
It should be "the mkostemp() returns"; and "to get" instead of "for
get". And in my opinion, "fchmod()" instead of plain "fchmod".
Enough description review, let's do code...
> diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc
> index dca468c..6910524 100644
> --- a/apt-pkg/contrib/fileutl.cc
> +++ b/apt-pkg/contrib/fileutl.cc
> @@ -946,9 +946,6 @@ bool FileFd::Open(string FileName,unsigned int const Mode,APT::Configuration::Co
> if ((Mode & Atomic) == Atomic)
> {
> Flags |= Replace;
> - char *name = strdup((FileName + ".XXXXXX").c_str());
> - TemporaryFileName = string(mktemp(name));
> - free(name);
> }
> else if ((Mode & (Exclusive | Create)) == (Exclusive | Create))
> {
> @@ -974,8 +971,27 @@ bool FileFd::Open(string FileName,unsigned int const Mode,APT::Configuration::Co
> else if_FLAGGED_SET(Atomic, O_EXCL);
> #undef if_FLAGGED_SET
>
> - if (TemporaryFileName.empty() == false)
> - iFd = open(TemporaryFileName.c_str(), fileflags, Perms);
> + if ((Mode & Atomic) == Atomic)
> + {
> + char *name = strdup((FileName + ".XXXXXX").c_str());
> +
> + TemporaryFileName = string(name);
This needs to be put below the next section, as the value of
name is changed by the call to mkostemp(), so we do not have
a final filename yet.
> +
> + // The file is created with permissions 0600, that is,
> + // read plus write for owner only.
> + if((iFd = mkostemp(name, fileflags)) == -1)
> + {
> + free(name);
> + return FileFdError("Could not create temporary file for %s", FileName.c_str());
> + }
I believe we want FileFdErrno instead of FileFdError.
> +
> + if(fchmod(iFd, Perms) == -1)
> + {
> + free(name);
> + return FileFdError("Could not assign permissions to temporary file %s with error %s", FileName.c_str(), strerror(errno));
> + }
> + free(name);
> + }
As above.
--
Julian Andres Klode - Debian Developer, Ubuntu Member
See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.
Reply to: