Re: [PATCH 1/2] server: Added server side named pipe read and write implementation and use it for message mode pipes.

[Sebastian Lackner]

On 28.02.2017 14:14, Jacek Caban wrote:
> - LIST_FOR_EACH_ENTRY( message, &pipe_end->message_queue, struct pipe_message, entry )
> -
>    avail += message->iosb->in_size - message->read_pos;
> - reply_size = min( reply_size, avail );
> -
> - if (avail)
> - {
> -
>    message = LIST_ENTRY( list_head(&pipe_end->message_queue), struct pipe_message, entry );
> -
>    reply_size = min( reply_size, message->iosb->in_size );
> - }

Isn't there a problem with this size check? Lets assume we have two messages
with 10 byte, for the first one the read_pos = 5. Then avail == 15, and reply_size
is clamped to 10 - but it will attempt to copy 10 bytes starting position 5?
> -
> - if (!(buffer = set_reply_data_size(FIELD_OFFSET( FILE_PIPE_PEEK_BUFFER, Data[reply_size] )))) return;
> - buffer->NamedPipeState    = 0;  /* FIXME */
> - buffer->ReadDataAvailable = avail;
> - buffer->NumberOfMessages  = 0;  /* FIXME */
> - buffer->MessageLength     = 0;  /* FIXME */
> - if (avail) memcpy( buffer->Data, (const char *)message->iosb->in_data + message->read_pos, reply_size );
>
> +}
> -

[PATCH 1/2] server: Added server side named pipe read and write implementation and use it for message mode pipes.

[Jacek Caban]

On 28.02.2017 19:36, Sebastian Lackner wrote:
> On 28.02.2017 14:14, Jacek Caban wrote:
> > - LIST_FOR_EACH_ENTRY( message, &pipe_end->message_queue, struct pipe_message, entry )
> > -
> >    avail += message->iosb->in_size - message->read_pos;
> > - reply_size = min( reply_size, avail );
> > -
> > - if (avail)
> > - {
> > -
> >    message = LIST_ENTRY( list_head(&pipe_end->message_queue), struct pipe_message, entry );
> > -
> >    reply_size = min( reply_size, message->iosb->in_size );
> > - }
>
> Isn't there a problem with this size check? Lets assume we have two messages
> with 10 byte, for the first one the read_pos = 5. Then avail == 15, and reply_size
> is clamped to 10 - but it will attempt to copy 10 bytes starting position 5?

Yes, you're right, good catch. It definitely needs more tests. I sent a
new version with extended tests.

Thanks,
Jacek