Re: [Spice-devel] [spice-streaming-agent PATCH] handle_stream_error: add comment for inheriting struct

[Frediano Ziglio]

> On Wed, 2018-03-07 at 13:56 +0100, Christophe de Dinechin wrote:
> > > On 7 Mar 2018, at 07:40, Frediano Ziglio  wrote:
> > > 
> > > > 
> > > > Introduced in 548577dc8adae1a558
> > > > 
> > > > Signed-off-by: Uri Lublin 
> > > > ---
> > > > src/spice-streaming-agent.cpp | 5 +
> > > > 1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/src/spice-streaming-agent.cpp
> > > > b/src/spice-streaming-agent.cpp
> > > > index b39782c..e25d47a 100644
> > > > --- a/src/spice-streaming-agent.cpp
> > > > +++ b/src/spice-streaming-agent.cpp
> > > > @@ -145,6 +145,11 @@ static void handle_stream_error(size_t len)
> > > >  
> > > > std::to_string(sizeof(StreamMsgNotifyError))
> > > >  + ")");
> > > > }
> > > > 
> > > > +// This struct inherits StreamMsgNotifyError. Its memory layout
> > > > is:
> > > > +// offset 0: StreamMsgNotifyError.error_code (a uint32_t)
> > > > +// offset 4: StreamMsgNotifyError.msg and also msg.msg (a
> > > > uint8_t[1024]).
> > > > +// Both StreamMsgNotifyError.msg and msg.msg point to the same
> > > > +// memory location (practically local msg overrides inherited
> > > > msg).
> > > > struct : StreamMsgNotifyError {
> > > > uint8_t msg[1024];
> > > > } msg;
> > > 
> > > When I sent my suggestion I didn't wanted to clash with inherited field,
> > > just didn't invented a new name.
> > > Maybe would be easier and more clear to rename to msg_buffer or similar
> > > to avoid the confusion?
> > 
> > Would fix the issue I pointed out. But as I mentioned there, gcc emkits a
> > warning that we access the array out of bounds (msg has zero length). You
> > can silence that using a temporary char * pointer.
> 
> Are you talking about the same thing as Frediano? IIUC:
> - Frediano says rename msg.msg to msg.msg_buffer and use that
> 
> - You talk about out of bounds warning for StreamMsgNotifyError::msg
> and casting it to a pointer to avoid that (so that we wouldn't need to
> inherit the struct at all)
> 
> To sum it up, what changes are requested?
> 
> - Fix msg.msg in a TBD way.
> 
> - If the message is too long, read it out instead of throwing an
> exception to prevent a DoS. (We can still get DoSed though if the
> server sends a neverending message or whatever?)
> 
> - If the inherited struct stays, name it? (I have to say I liked the
> unnamed struct in local scope as it limited it's usage exactly to the
> one 'msg' variable...)
> 
> Cheers,
> Lukas
> 

I think something like

--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -159,13 +159,13 @@ static void handle_stream_error(size_t len)
 }

 struct : StreamMsgNotifyError {
-uint8_t msg[1024];
+uint8_t msg_buffer[1024];
 } msg;

 size_t len_to_read = std::min(len, sizeof(msg) - 1);

 read_all(, len_to_read);
-msg.msg[len_to_read - sizeof(StreamMsgNotifyError)] = '\0';
+((uint8_t *) )[len_to_read] = '\0';

 syslog(LOG_ERR, "Received NotifyError message from the server: %d - %s\n",
 msg.error_code, msg.msg);


But the terminating like is relatively hackish.
Also Christophe asked for a name for the "msg" structure but didn't
come with something sensible.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-streaming-agent PATCH] handle_stream_error: add comment for inheriting struct

[Lukáš Hrázký]

On Wed, 2018-03-07 at 13:56 +0100, Christophe de Dinechin wrote:
> > On 7 Mar 2018, at 07:40, Frediano Ziglio  wrote:
> > 
> > > 
> > > Introduced in 548577dc8adae1a558
> > > 
> > > Signed-off-by: Uri Lublin 
> > > ---
> > > src/spice-streaming-agent.cpp | 5 +
> > > 1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > > index b39782c..e25d47a 100644
> > > --- a/src/spice-streaming-agent.cpp
> > > +++ b/src/spice-streaming-agent.cpp
> > > @@ -145,6 +145,11 @@ static void handle_stream_error(size_t len)
> > >  
> > > std::to_string(sizeof(StreamMsgNotifyError))
> > >  + ")");
> > > }
> > > 
> > > +// This struct inherits StreamMsgNotifyError. Its memory layout is:
> > > +// offset 0: StreamMsgNotifyError.error_code (a uint32_t)
> > > +// offset 4: StreamMsgNotifyError.msg and also msg.msg (a
> > > uint8_t[1024]).
> > > +// Both StreamMsgNotifyError.msg and msg.msg point to the same
> > > +// memory location (practically local msg overrides inherited msg).
> > > struct : StreamMsgNotifyError {
> > > uint8_t msg[1024];
> > > } msg;
> > 
> > When I sent my suggestion I didn't wanted to clash with inherited field,
> > just didn't invented a new name.
> > Maybe would be easier and more clear to rename to msg_buffer or similar
> > to avoid the confusion?
> 
> Would fix the issue I pointed out. But as I mentioned there, gcc emkits a 
> warning that we access the array out of bounds (msg has zero length). You can 
> silence that using a temporary char * pointer.

Are you talking about the same thing as Frediano? IIUC:
- Frediano says rename msg.msg to msg.msg_buffer and use that

- You talk about out of bounds warning for StreamMsgNotifyError::msg
and casting it to a pointer to avoid that (so that we wouldn't need to
inherit the struct at all)

To sum it up, what changes are requested?

- Fix msg.msg in a TBD way.

- If the message is too long, read it out instead of throwing an
exception to prevent a DoS. (We can still get DoSed though if the
server sends a neverending message or whatever?)

- If the inherited struct stays, name it? (I have to say I liked the
unnamed struct in local scope as it limited it's usage exactly to the
one 'msg' variable...)

Cheers,
Lukas

> > 
> > Frediano
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-streaming-agent PATCH] handle_stream_error: add comment for inheriting struct

[Christophe de Dinechin]

> On 7 Mar 2018, at 07:40, Frediano Ziglio  wrote:
> 
>> 
>> Introduced in 548577dc8adae1a558
>> 
>> Signed-off-by: Uri Lublin 
>> ---
>> src/spice-streaming-agent.cpp | 5 +
>> 1 file changed, 5 insertions(+)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index b39782c..e25d47a 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -145,6 +145,11 @@ static void handle_stream_error(size_t len)
>>  std::to_string(sizeof(StreamMsgNotifyError))
>>  + ")");
>> }
>> 
>> +// This struct inherits StreamMsgNotifyError. Its memory layout is:
>> +// offset 0: StreamMsgNotifyError.error_code (a uint32_t)
>> +// offset 4: StreamMsgNotifyError.msg and also msg.msg (a
>> uint8_t[1024]).
>> +// Both StreamMsgNotifyError.msg and msg.msg point to the same
>> +// memory location (practically local msg overrides inherited msg).
>> struct : StreamMsgNotifyError {
>> uint8_t msg[1024];
>> } msg;
> 
> When I sent my suggestion I didn't wanted to clash with inherited field,
> just didn't invented a new name.
> Maybe would be easier and more clear to rename to msg_buffer or similar
> to avoid the confusion?

Would fix the issue I pointed out. But as I mentioned there, gcc emkits a 
warning that we access the array out of bounds (msg has zero length). You can 
silence that using a temporary char * pointer.

> 
> Frediano
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-streaming-agent PATCH] handle_stream_error: add comment for inheriting struct

[Frediano Ziglio]

> 
> Introduced in 548577dc8adae1a558
> 
> Signed-off-by: Uri Lublin 
> ---
>  src/spice-streaming-agent.cpp | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index b39782c..e25d47a 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -145,6 +145,11 @@ static void handle_stream_error(size_t len)
>   std::to_string(sizeof(StreamMsgNotifyError))
>   + ")");
>  }
>  
> +// This struct inherits StreamMsgNotifyError. Its memory layout is:
> +// offset 0: StreamMsgNotifyError.error_code (a uint32_t)
> +// offset 4: StreamMsgNotifyError.msg and also msg.msg (a
> uint8_t[1024]).
> +// Both StreamMsgNotifyError.msg and msg.msg point to the same
> +// memory location (practically local msg overrides inherited msg).
>  struct : StreamMsgNotifyError {
>  uint8_t msg[1024];
>  } msg;

When I sent my suggestion I didn't wanted to clash with inherited field,
just didn't invented a new name.
Maybe would be easier and more clear to rename to msg_buffer or similar
to avoid the confusion?

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel