diff mbox series

[1/4] xen/console: Properly buffer domU output when using CONSOLEIO_write

Message ID 20190402164238.1815-2-julien.grall@arm.com (mailing list archive)
State New, archived
Headers show
Series xen/console: Bug fixes and doc improvement | expand

Commit Message

Julien Grall April 2, 2019, 4:42 p.m. UTC
The output will be buffered if the buffer provided by the DomU does not
contain a newline. This can also happen if buffer provided by DomU is
split in multiple part (Xen can only process 127 characters at the time).

As Xen will remove any non-printable characters, the output buffer may
be smaller than the buffer provided. However, Xen will buffer using the
original length. This means that the NUL character and garbagge will be
copied in the internal buffer.

Once the newline is found or the internal buffer is full, only part of
the internal buffer will end up to be printed.

An easy way to reproduce it is:

HYPERVISOR_consoleio(CONSOLEIO_write, "\33", 1);
HYPERVISOR_consoleio(CONSOLEIO_write, "d", 1);
HYPERVISOR_consoleio(CONSOLEIO_write, "\n", 1);

In the current code, the character 'd' will not be printed.

This problem can be solved by computing the size of the output buffer
(i.e the buffer without the non-printable characters).

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

I is possible to compute (kout - kbuf) only once. I didn't do it because
I wasn't able to find a good name.
---
 xen/drivers/char/console.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Wei Liu April 3, 2019, 11:41 a.m. UTC | #1
On Tue, Apr 02, 2019 at 05:42:35PM +0100, Julien Grall wrote:
> The output will be buffered if the buffer provided by the DomU does not
> contain a newline. This can also happen if buffer provided by DomU is
> split in multiple part (Xen can only process 127 characters at the time).
> 
> As Xen will remove any non-printable characters, the output buffer may
> be smaller than the buffer provided. However, Xen will buffer using the
> original length. This means that the NUL character and garbagge will be
> copied in the internal buffer.
> 
> Once the newline is found or the internal buffer is full, only part of
> the internal buffer will end up to be printed.
> 
> An easy way to reproduce it is:
> 
> HYPERVISOR_consoleio(CONSOLEIO_write, "\33", 1);
> HYPERVISOR_consoleio(CONSOLEIO_write, "d", 1);
> HYPERVISOR_consoleio(CONSOLEIO_write, "\n", 1);
> 
> In the current code, the character 'd' will not be printed.
> 
> This problem can be solved by computing the size of the output buffer
> (i.e the buffer without the non-printable characters).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>
Julien Grall April 9, 2019, 10:25 a.m. UTC | #2
Hi,

On 03/04/2019 12:41, Wei Liu wrote:
> On Tue, Apr 02, 2019 at 05:42:35PM +0100, Julien Grall wrote:
>> The output will be buffered if the buffer provided by the DomU does not
>> contain a newline. This can also happen if buffer provided by DomU is
>> split in multiple part (Xen can only process 127 characters at the time).
>>
>> As Xen will remove any non-printable characters, the output buffer may
>> be smaller than the buffer provided. However, Xen will buffer using the
>> original length. This means that the NUL character and garbagge will be
>> copied in the internal buffer.
>>
>> Once the newline is found or the internal buffer is full, only part of
>> the internal buffer will end up to be printed.
>>
>> An easy way to reproduce it is:
>>
>> HYPERVISOR_consoleio(CONSOLEIO_write, "\33", 1);
>> HYPERVISOR_consoleio(CONSOLEIO_write, "d", 1);
>> HYPERVISOR_consoleio(CONSOLEIO_write, "\n", 1);
>>
>> In the current code, the character 'd' will not be printed.
>>
>> This problem can be solved by computing the size of the output buffer
>> (i.e the buffer without the non-printable characters).
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

I have committed this patch. The rest of the series need to be respined.

Cheers,

>
diff mbox series

Patch

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 5f0f54201b..9bbcb0f57a 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -592,11 +592,11 @@  static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
                 guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
                 cd->pbuf_idx = 0;
             }
-            else if ( cd->pbuf_idx + kcount < (DOMAIN_PBUF_SIZE - 1) )
+            else if ( cd->pbuf_idx + (kout - kbuf) < (DOMAIN_PBUF_SIZE - 1) )
             {
                 /* buffer the output until a newline */
-                memcpy(cd->pbuf + cd->pbuf_idx, kbuf, kcount);
-                cd->pbuf_idx += kcount;
+                memcpy(cd->pbuf + cd->pbuf_idx, kbuf, kout - kbuf);
+                cd->pbuf_idx += (kout - kbuf);
             }
             else
             {