Received-SPF: softfail (google.com: domain of transitioning linux-bluetooth-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220329052801.28748-1-wangxinpeng@uniontech.com>
In-Reply-To: <20220329052801.28748-1-wangxinpeng@uniontech.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Wed, 30 Mar 2022 13:56:57 -0700
Message-ID: <CABBYNZKDdRLEk7NEHoZ_uJV2cUNm9qHuWvaiqDotWpSL=LnLQg@mail.gmail.com>
Subject: Re: [PATCH v4] obexd: Fix can't receive small files sent by windows
To:     xinpeng wang <wangxinpeng@uniontech.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Mon, Mar 28, 2022 at 11:02 PM xinpeng wang <wangxinpeng@uniontech.com> wrote:
>
> When obexd receives a file, it parses the filename and other parameters
> when processing the packet for the first time, store filename in
> obex_session and emit the dbus signal, The signal will be pending first.
> then when this file is received, transfer_complete reset obex_session
> is called.

So if I understand you correctly the problem is that Status does
transit directly to complete and end up swallowing the active state?
That is not necessarily a bug since the file would still be saved.

> When using a computer with Windows 10 installed to send a file to bluez,
> obexd will read the data through read_stream; if it is a small file, the
> data processed for the first time is marked as final, and
> transfer_complete reset obex_session will be called when the data is
> processed for the first time. At this point, the dbus signal is still
> pending, and the dbus method that requests the file path has not been
> processed. This will cause the upper application to not be able to
> transfer files from the cache directory to the directory specified by
> the user.

This doesn't sound right, we have chk_put callback that calls into
manager_request_authorization and that blocks untils the
agent->auth_pending responds, so it sounds like the agent is not
responding in time or windows is using ftp instead of opp? Well ftp
doesn't even use AuthorizePush, I'm also confused about the mention of
a cache directory and user specified directory, there are no such
things there only the root directory option which is used as default
location and AuthorizePush can opt to store in a different location if
necessary, but the transfer doesn't start until the agent reply
AuthorizePush or at least that what the code assumes with:

/* Workaround: process events while agent doesn't reply */
while (agent && agent->auth_pending)
g_main_context_iteration(NULL, TRUE);

> To solve this problem, emit Filename's dbus signal and force it when
> status=active.
>
> Ways to reproduce the problem:
> 1. Use the computer with windows 10 installed to send a small file to
> the computer with ubuntu installed;
> 2. file size < 10k;
> 3. After sending, in most cases, the file is located in the
> ~/.cache/obexd/ directory. Normally, the file should be located in the
> ~/Download directory.

Is ~/.cache/obexd its passed as -r/--root to obexd? Could it be that
the agent doesn't respond for some reason? e.g. the transfer status is
not active?

> To fix this, after applying this commit, it also needs to be modified by
> the upper-level application. Modified to read Filename from dbus signal
> if there is Filename in dbus signal. Otherwise, use the dbus method to
> request Filename.

The method is used to accept the request, so something else is broken
if the file ends up being accepted before the user is able to accept
it, also it is probably a security breach if small files can bypass
the user authorization.

> ---
> v4: fix COMMIT_LOG_LONG_LINE error of checkpatch
>
>  obexd/src/manager.c | 6 +++++-
>  obexd/src/obex.c    | 1 +
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/obexd/src/manager.c b/obexd/src/manager.c
> index 01741fe62..2c180dc44 100644
> --- a/obexd/src/manager.c
> +++ b/obexd/src/manager.c
> @@ -533,8 +533,12 @@ void manager_emit_transfer_property(struct obex_transfer *transfer,
>  void manager_emit_transfer_started(struct obex_transfer *transfer)
>  {
>         transfer->status = TRANSFER_STATUS_ACTIVE;
> +       if (!transfer->path)
> +               return;
>
> -       manager_emit_transfer_property(transfer, "Status");
> +       g_dbus_emit_property_changed_full(connection, transfer->path,
> +                                       TRANSFER_INTERFACE, "Status",
> +                                       G_DBUS_PROPERTY_CHANGED_FLAG_FLUSH);
>  }
>
>  static void emit_transfer_completed(struct obex_transfer *transfer,
> diff --git a/obexd/src/obex.c b/obexd/src/obex.c
> index 3a68fd66c..c0d9e160a 100644
> --- a/obexd/src/obex.c
> +++ b/obexd/src/obex.c
> @@ -720,6 +720,7 @@ int obex_put_stream_start(struct obex_session *os, const char *filename)
>                 manager_emit_transfer_property(os->service_data, "Size");
>
>         os->path = g_strdup(filename);
> +       manager_emit_transfer_property(os->service_data, "Filename");
>
>         return 0;
>  }
> --
> 2.20.1
>
>
>


-- 
Luiz Augusto von Dentz