2012-05-29 12:11:38

by Luiz Augusto von Dentz

Subject: [PATCH obexd] client: Fix not checking if destination exist while renaming file

From: Luiz Augusto von Dentz <[email protected]>

rename return an error if the destination directory doesn't exist, so
in case it doesn't exist it should be created before calling rename.
---
client/transfer.c | 27 +++++++++++++++++++++++++--
1 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/client/transfer.c b/client/transfer.c
index 3065c9c..772834f 100644
--- a/client/transfer.c
+++ b/client/transfer.c
@@ -715,11 +715,34 @@ int obc_transfer_set_filename(struct obc_transfer *transfer,
const char *filename)
{
int err;
+ struct stat st;
+ char *dirname;
+ gboolean dir = FALSE;
+
+ dirname = g_path_get_dirname(filename);
+ if (stat(dirname, &st) < 0) {
+ if (errno != ENOENT) {
+ error("stat(): %s (%d)", strerror(errno), errno);
+ return -errno;
+ }
+
+ if (mkdir(dirname, 0755) < 0) {
+ error("mkdir(): %s (%d)", strerror(errno), errno);
+ return -errno;
+ }
+
+ dir = TRUE;
+ }

err = rename(transfer->filename, filename);
if (err < 0) {
- error("rename(): %s (%d)", strerror(errno), errno);
- return -errno;
+ err = -errno;
+
+ if (dir)
+ rmdir(dirname);
+
+ error("rename(): %s (%d)", strerror(-err), -err);
+ return -err;
}

g_free(transfer->filename);
--
1.7.7.6

2012-05-29 18:08:38

by Luiz Augusto von Dentz

Subject: Re: [PATCH obexd] client: Fix not checking if destination exist while renaming file

Hi Vinicius,

On Tue, May 29, 2012 at 5:25 PM, Vinicius Costa Gomes
<[email protected]> wrote:
> Hi Luiz,
>
> On 15:11 Tue 29 May, Luiz Augusto von Dentz wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> rename return an error if the destination directory doesn't exist, so
>> in case it doesn't exist it should be created before calling rename.
>> ---
>> ?client/transfer.c | ? 27 +++++++++++++++++++++++++--
>> ?1 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/client/transfer.c b/client/transfer.c
>> index 3065c9c..772834f 100644
>> --- a/client/transfer.c
>> +++ b/client/transfer.c
>> @@ -715,11 +715,34 @@ int obc_transfer_set_filename(struct obc_transfer *transfer,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? const char *filename)
>> ?{
>> ? ? ? int err;
>> + ? ? struct stat st;
>> + ? ? char *dirname;
>> + ? ? gboolean dir = FALSE;
>> +
>> + ? ? dirname = g_path_get_dirname(filename);
>> + ? ? if (stat(dirname, &st) < 0) {
>> + ? ? ? ? ? ? if (errno != ENOENT) {
>> + ? ? ? ? ? ? ? ? ? ? error("stat(): %s (%d)", strerror(errno), errno);
>> + ? ? ? ? ? ? ? ? ? ? return -errno;
>> + ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? if (mkdir(dirname, 0755) < 0) {
>
> For example, if my /tmp is empty, and "dirname" is "/tmp/foo/bar", this
> would fail, right? i.e. mkdir() isn't able to create two (or more) levels
> at a time.
>
> So for consistency, I would prefer if the error that the directory
> doesn't exist is returned, and the user creates the directory himself.
> This is mostly for FTP, right?

This is a regression when the Agent returns a different path to save
the file it used to work, but since we are going to remove the agent
maybe this function doesn't make sense anymore so the application need
to set the filename upfront.


--
Luiz Augusto von Dentz

2012-05-29 14:25:37

by Vinicius Costa Gomes

Subject: Re: [PATCH obexd] client: Fix not checking if destination exist while renaming file

Hi Luiz,

On 15:11 Tue 29 May, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> rename return an error if the destination directory doesn't exist, so
> in case it doesn't exist it should be created before calling rename.
> ---
> client/transfer.c | 27 +++++++++++++++++++++++++--
> 1 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/client/transfer.c b/client/transfer.c
> index 3065c9c..772834f 100644
> --- a/client/transfer.c
> +++ b/client/transfer.c
> @@ -715,11 +715,34 @@ int obc_transfer_set_filename(struct obc_transfer *transfer,
> const char *filename)
> {
> int err;
> + struct stat st;
> + char *dirname;
> + gboolean dir = FALSE;
> +
> + dirname = g_path_get_dirname(filename);
> + if (stat(dirname, &st) < 0) {
> + if (errno != ENOENT) {
> + error("stat(): %s (%d)", strerror(errno), errno);
> + return -errno;
> + }
> +
> + if (mkdir(dirname, 0755) < 0) {

For example, if my /tmp is empty, and "dirname" is "/tmp/foo/bar", this
would fail, right? i.e. mkdir() isn't able to create two (or more) levels
at a time.

So for consistency, I would prefer if the error that the directory
doesn't exist is returned, and the user creates the directory himself.
This is mostly for FTP, right?

> + error("mkdir(): %s (%d)", strerror(errno), errno);
> + return -errno;
> + }
> +
> + dir = TRUE;
> + }
>
> err = rename(transfer->filename, filename);
> if (err < 0) {
> - error("rename(): %s (%d)", strerror(errno), errno);
> - return -errno;
> + err = -errno;
> +
> + if (dir)
> + rmdir(dirname);
> +
> + error("rename(): %s (%d)", strerror(-err), -err);
> + return -err;
> }
>
> g_free(transfer->filename);
> --
> 1.7.7.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html


Cheers,
--
Vinicius