Hi,
On Thu, Jun 27, 2024 at 12:24 PM Pauli Virtanen <pav@xxxxxx> wrote:
>
> Hi,
>
> ke, 2024-06-26 kello 11:40 +0530, Amisha Jain kirjoitti:
> > Hi,
> >
> > Correct! The problem here is manager_emit_transfer_property() is
> > expecting the structure of type 'obex_transfer' and we are passing the
> > structure of session type which will be type mismatch and inappropriate
> > values will be populated in further calls in code. Hence property "Size"
> > will never emit on console(obexctl) as it is not set properly and might
> > cause crash/disconnection.
> >
> > > diff --git a/obexd/src/obex.c b/obexd/src/obex.c
> > > index a4bae857f..ed219d3e7 100644
> > > --- a/obexd/src/obex.c
> > > +++ b/obexd/src/obex.c
> > > @@ -779,6 +779,9 @@ int obex_put_stream_start(struct obex_session
> > *os, const char *filename)
> > > return err;
> > > }
> > >
> > > + if (os->size != OBJECT_SIZE_DELETE && os->size !=
> > OBJECT_SIZE_UNKNOWN)
> > > + manager_emit_transfer_property(os->service_data, "Size");
> > > +
> > > os->path = g_strdup(filename);
> > >
> >
> >
> > One way to resolve this issue is to add the additional field in
> > 'mas_session' so it can cast to struct 'obex_transfer'. We are adding
> > new field 'char *path' as only transfer->path will be invoked and passed
> > further.
>
> Yes, thank you for clarification.
>
> Although I'm not BlueZ maintainer, I think relying on type punning
> mas_session and obex_transfer by adding fields so that their memory
> representation is partly the same is not very good practice. Other
> plugins like ftp.c seem also have similar issue.
Yeah, it clearly was not intended to be that way.
> One could consider some ways to avoid doing this, e.g. move the
>
> if (os->size != OBJECT_SIZE_DELETE && os->size != OBJECT_SIZE_UNKNOWN)
> manager_emit_transfer_property(os->service_data, "Size");
>
> after each call to obex_put_stream_start in plugins/*.c
> client/mns.c. For plugins where there's no transfer, it maybe can be
> omitted.
I think this was intended to be called directly by the plugins
otherwise we need to always create transfer objects for each object
being transferred and that be set as os->service_data which obviously
isn't the case here.
> >
> > void manager_emit_transfer_property(struct obex_transfer *transfer,
> > char *name)
> > {
> > if (!transfer->path)
> > return;
> >
> > g_dbus_emit_property_changed(connection, transfer->path,
> > TRANSFER_INTERFACE, name);
> > }
> >
> > >> Signed-off-by: Amisha Jain <quic_amisjain@xxxxxxxxxxx>
> > >> ---
> > >> obexd/plugins/mas.c | 4 +++-
> > >> 1 file changed, 3 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/obexd/plugins/mas.c b/obexd/plugins/mas.c
> > >> index 10b972d65..71bf12ad3 100644
> > >> --- a/obexd/plugins/mas.c
> > >> +++ b/obexd/plugins/mas.c
> > >> @@ -51,6 +51,8 @@
> > >> #define ML_BODY_END "</MAP-msg-listing>"
> > >>
> > >> struct mas_session {
> > >> + uint8_t notification_status;
> > >> + char *path;
> > >> struct mas_request *request;
> > >> void *backend_data;
> > >> gboolean finished;
> > >> @@ -59,7 +61,6 @@ struct mas_session {
> > >> GObexApparam *inparams;
> > >> GObexApparam *outparams;
> > >> gboolean ap_sent;
> > >> - uint8_t notification_status;
> > >> };
> > >>
> > >> static const uint8_t MAS_TARGET[TARGET_SIZE] = {
> > >> @@ -125,6 +126,7 @@ static void *mas_connect(struct obex_session
> > *os, int *err)
> > >> goto failed;
> > >>
> > >> manager_register_session(os);
> > >> + mas->path = NULL;
> > >
> >
> > There is no transfer already registered for mas during connection, so
> > setting the path to NULL.
> >
> > On 6/25/2024 9:09 PM, Pauli Virtanen wrote:
> > > Hi,
> > >
> > > ti, 2024-06-25 kello 12:05 +0530, Amisha Jain kirjoitti:
> > > > Update the 'mas_session' structure such that
> > > > manager_emit_transfer_property(os->service_data, "Size")
> > > > will get the proper structure in arguments as
> > > > expected like structure 'obex_transfer' and transfer->path
> > > > won't be populated with inappropriate value.
> > > >
> > > > As there is no new transfer registered during mas connect,
> > > > hence setting the path to NULL to avoid invoking the
> > > > g_dbus_emit_property_changed() property.
> > > >
> > > > Signed-off-by: Amisha Jain <quic_amisjain@xxxxxxxxxxx>
> > > > ---
> > > > obexd/plugins/mas.c | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/obexd/plugins/mas.c b/obexd/plugins/mas.c
> > > > index 10b972d65..71bf12ad3 100644
> > > > --- a/obexd/plugins/mas.c
> > > > +++ b/obexd/plugins/mas.c
> > > > @@ -51,6 +51,8 @@
> > > > #define ML_BODY_END "</MAP-msg-listing>"
> > > >
> > > > struct mas_session {
> > > > + uint8_t notification_status;
> > > > + char *path;
> > > > struct mas_request *request;
> > > > void *backend_data;
> > > > gboolean finished;
> > > > @@ -59,7 +61,6 @@ struct mas_session {
> > > > GObexApparam *inparams;
> > > > GObexApparam *outparams;
> > > > gboolean ap_sent;
> > > > - uint8_t notification_status;
> > > > };
> > > >
> > > > static const uint8_t MAS_TARGET[TARGET_SIZE] = {
> > > > @@ -125,6 +126,7 @@ static void *mas_connect(struct obex_session *os, int *err)
> > > > goto failed;
> > > >
> > > > manager_register_session(os);
> > > > + mas->path = NULL;
> > >
> > > Maybe the problem here is that the change in commit bb160515185e
> > > ("obexd: Emit Size property of transfer after open()") is not right?
> > >
> > > diff --git a/obexd/src/obex.c b/obexd/src/obex.c
> > > index a4bae857f..ed219d3e7 100644
> > > --- a/obexd/src/obex.c
> > > +++ b/obexd/src/obex.c
> > > @@ -779,6 +779,9 @@ int obex_put_stream_start(struct obex_session *os, const char *filename)
> > > return err;
> > > }
> > >
> > > + if (os->size != OBJECT_SIZE_DELETE && os->size != OBJECT_SIZE_UNKNOWN)
> > > + manager_emit_transfer_property(os->service_data, "Size");
> > > +
> > > os->path = g_strdup(filename);
> > >
> > > This casts os->service_data to obex_transfer which IIUC does not work
> > > for most the plugins, as it's the session struct.
> > >
> > > Maybe plugins can emit the transfer property change in their open()
> > > callback, for the plugins where it makes sense?
> > >
> > > >
> > > > return mas;
> > > >
> > >
>
> --
> Pauli Virtanen
>
--
Luiz Augusto von Dentz
