[PATCH v2 1/3] console: don't count newlines twice in bytes written

[PATCH v2 1/3] console: don't count newlines twice in bytes written

[Ahmad Fatoum]

Both the PBL and simple console only return number of input bytes, not
number of bytes actually written out. These differ, because each LF is
converted to CRLF pairs.

The behavior of not counting actual written out characters is more sensible,
because otherwise callers interested in finding out if all bytes have been
written (e.g. to avoid incomplete writes with ratp) would need to keep count
of all line feeds in the string.
Therefore change the normal console to behave like its less featureful
brethren.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
Changes in v2:
	New commit.
---
 common/console.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/common/console.c b/common/console.c
index 47ccf2e54de0..52162df23b04 100644
--- a/common/console.c
+++ b/common/console.c
@@ -258,10 +258,9 @@ static int __console_puts(struct console_device *cdev, const char *s)
 	int n = 0;
 
 	while (*s) {
-		if (*s == '\n') {
+		if (*s == '\n')
 			cdev->putc(cdev, '\r');
-			n++;
-		}
+
 		cdev->putc(cdev, *s);
 		n++;
 		s++;
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 2/3] ratp: return 0 bytes written from puts if busy

[Ahmad Fatoum]

Prior behavior was to wrongly report all bytes written if enqueueing wasn't
possible at the time. Instead we should either return 0 or an error code if
users need to retry. write(2) returns 0 in such cases. Follow suit.

As no current users run puts in a loop, this has no effect for now.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
Changes in v2:
	New commit.
---
 common/ratp/ratp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c
index 9aea1786d684..8ac7dc98b6f8 100644
--- a/common/ratp/ratp.c
+++ b/common/ratp/ratp.c
@@ -267,7 +267,7 @@ static int ratp_console_puts(struct console_device *cdev, const char *s)
 	len = strlen(s);
 
 	if (ratp_busy(&ctx->ratp))
-		return len;
+		return 0;
 
 	kfifo_put(ctx->console_transmit_fifo, s, len);
 
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 3/3] console: fix out-of-bounds read in dputc(/dev/*, ...)

[Ahmad Fatoum]

Trying to output a single character via
	echo -a /dev/serial0-1
currently results in garbage output after the newline, because console.c's
fops_write discards the buffer length and passes the buffer to
(struct cdev)::puts which only handles NUL-terminated strings.

Fix this by amending (struct cdev)::puts with a new nbytes parameter,
which is correctly propagated.

Fixes: b4f55fcf35 ("console: expose consoles in devfs")
Cc: Bastian Krause <bst@pengutronix.de>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
Changes in v2:
	- fixed commit hash in Fixes:, by Bastian
	- fixed possible incomplete writes in __console_puts
	- rebased on top of two potential bug fixes
---
 common/console.c            | 14 +++++++-------
 common/ratp/ratp.c          | 10 ++++------
 drivers/serial/efi-stdio.c  |  5 +++--
 drivers/serial/serial_efi.c |  5 +++--
 fs/pstore/platform.c        |  7 ++++---
 include/console.h           |  2 +-
 6 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/common/console.c b/common/console.c
index 52162df23b04..ce4122fa76f1 100644
--- a/common/console.c
+++ b/common/console.c
@@ -253,19 +253,19 @@ static void console_set_stdoutpath(struct console_device *cdev)
 	free(str);
 }
 
-static int __console_puts(struct console_device *cdev, const char *s)
+static int __console_puts(struct console_device *cdev, const char *s,
+			  size_t nbytes)
 {
-	int n = 0;
+	size_t i;
 
-	while (*s) {
+	for (i = 0; i < nbytes; i++) {
 		if (*s == '\n')
 			cdev->putc(cdev, '\r');
 
 		cdev->putc(cdev, *s);
-		n++;
 		s++;
 	}
-	return n;
+	return i;
 }
 
 static int fops_open(struct cdev *cdev, unsigned long flags)
@@ -297,7 +297,7 @@ static ssize_t fops_write(struct cdev* dev, const void* buf, size_t count,
 {
 	struct console_device *priv = dev->priv;
 
-	priv->puts(priv, buf);
+	priv->puts(priv, buf, count);
 
 	return 0;
 }
@@ -544,7 +544,7 @@ int console_puts(unsigned int ch, const char *str)
 	if (initialized == CONSOLE_INIT_FULL) {
 		for_each_console(cdev) {
 			if (cdev->f_active & ch) {
-				n = cdev->puts(cdev, str);
+				n = cdev->puts(cdev, str, strlen(str));
 			}
 		}
 		return n;
diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c
index 8ac7dc98b6f8..8f989515e485 100644
--- a/common/ratp/ratp.c
+++ b/common/ratp/ratp.c
@@ -259,19 +259,17 @@ static int ratp_console_tstc(struct console_device *cdev)
 	return kfifo_len(ctx->console_recv_fifo) ? 1 : 0;
 }
 
-static int ratp_console_puts(struct console_device *cdev, const char *s)
+static int ratp_console_puts(struct console_device *cdev, const char *s,
+			     size_t nbytes)
 {
 	struct ratp_ctx *ctx = container_of(cdev, struct ratp_ctx, ratp_console);
-	int len = 0;
-
-	len = strlen(s);
 
 	if (ratp_busy(&ctx->ratp))
 		return 0;
 
-	kfifo_put(ctx->console_transmit_fifo, s, len);
+	kfifo_put(ctx->console_transmit_fifo, s, nbytes);
 
-	return len;
+	return nbytes;
 }
 
 static void ratp_console_putc(struct console_device *cdev, char c)
diff --git a/drivers/serial/efi-stdio.c b/drivers/serial/efi-stdio.c
index 0703f727e766..2ca89fa4f861 100644
--- a/drivers/serial/efi-stdio.c
+++ b/drivers/serial/efi-stdio.c
@@ -243,12 +243,13 @@ static int efi_process_key(struct efi_console_priv *priv, const char *inp)
 	return 1;
 }
 
-static int efi_console_puts(struct console_device *cdev, const char *s)
+static int efi_console_puts(struct console_device *cdev, const char *s,
+			    size_t nbytes)
 {
 	struct efi_console_priv *priv = to_efi(cdev);
 	int n = 0;
 
-	while (*s) {
+	while (nbytes--) {
 		if (*s == 27) {
 			priv->efi_console_buffer[n] = 0;
 			priv->out->output_string(priv->out,
diff --git a/drivers/serial/serial_efi.c b/drivers/serial/serial_efi.c
index f0a2b22c2bc2..667d51f622ec 100644
--- a/drivers/serial/serial_efi.c
+++ b/drivers/serial/serial_efi.c
@@ -130,13 +130,14 @@ static void efi_serial_putc(struct console_device *cdev, char c)
 	serial->write(serial, &buffersize, &c);
 }
 
-static int efi_serial_puts(struct console_device *cdev, const char *s)
+static int efi_serial_puts(struct console_device *cdev, const char *s,
+			   size_t nbytes)
 {
 	struct efi_serial_port *uart = to_efi_serial_port(cdev);
 	struct efi_serial_io_protocol *serial = uart->serial;
 	uint32_t control;
 	efi_status_t efiret;
-	unsigned long buffersize = strlen(s) * sizeof(char);
+	unsigned long buffersize = nbytes;
 
 	do {
 		efiret = serial->getcontrol(serial, &control);
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 755363c30999..601bfca6b025 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -67,10 +67,11 @@ static void pstore_console_write(const char *s, unsigned c)
 	}
 }
 
-static int pstore_console_puts(struct console_device *cdev, const char *s)
+static int pstore_console_puts(struct console_device *cdev, const char *s,
+			       size_t nbytes)
 {
-	pstore_console_write(s, strlen(s));
-	return strlen(s);
+	pstore_console_write(s, nbytes);
+	return nbytes;
 }
 
 static void pstore_console_putc(struct console_device *cdev, char c)
diff --git a/include/console.h b/include/console.h
index 673921331db7..1d86a584a7f2 100644
--- a/include/console.h
+++ b/include/console.h
@@ -42,7 +42,7 @@ struct console_device {
 
 	int (*tstc)(struct console_device *cdev);
 	void (*putc)(struct console_device *cdev, char c);
-	int (*puts)(struct console_device *cdev, const char *s);
+	int (*puts)(struct console_device *cdev, const char *s, size_t nbytes);
 	int  (*getc)(struct console_device *cdev);
 	int (*setbrg)(struct console_device *cdev, int baudrate);
 	void (*flush)(struct console_device *cdev);
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 2/3] ratp: return 0 bytes written from puts if busy

[Sascha Hauer]

On Wed, Jul 31, 2019 at 12:21:42PM +0200, Ahmad Fatoum wrote:
> Prior behavior was to wrongly report all bytes written if enqueueing wasn't
> possible at the time. Instead we should either return 0 or an error code if
> users need to retry. write(2) returns 0 in such cases. Follow suit.
> 
> As no current users run puts in a loop, this has no effect for now.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> Changes in v2:
> 	New commit.
> ---
>  common/ratp/ratp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c
> index 9aea1786d684..8ac7dc98b6f8 100644
> --- a/common/ratp/ratp.c
> +++ b/common/ratp/ratp.c
> @@ -267,7 +267,7 @@ static int ratp_console_puts(struct console_device *cdev, const char *s)
>  	len = strlen(s);
>  
>  	if (ratp_busy(&ctx->ratp))
> -		return len;
> +		return 0;

I'm not sure if this return value is ever used for something useful,
not sure how relevant this is. ratp_busy() however returns true when
it's called from inside the ratp code. This is necessary so that we
don't get stuck in an endless loop. If we start returning 0 for
"no characters sent" how should code evaluating this return value
react? Retrying it until all characters are sent obviously is not an
option.

I think the current behaviour of just returning 'len' is correct.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/3] console: don't count newlines twice in bytes written

[Sascha Hauer]

On Wed, Jul 31, 2019 at 12:21:41PM +0200, Ahmad Fatoum wrote:
> Both the PBL and simple console only return number of input bytes, not
> number of bytes actually written out. These differ, because each LF is
> converted to CRLF pairs.
> 
> The behavior of not counting actual written out characters is more sensible,
> because otherwise callers interested in finding out if all bytes have been
> written (e.g. to avoid incomplete writes with ratp) would need to keep count
> of all line feeds in the string.
> Therefore change the normal console to behave like its less featureful
> brethren.

According to "man puts" puts() returns a non negative number on success.
I can't find a place where it's claimed that we have to return the
number of characters. I also can't find a place where the return value
of any puts like function in barebox is ever evaluated. Given that it
might make more sense to just return zero in the absense of an error.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 2/3] ratp: return 0 bytes written from puts if busy

[Ahmad Fatoum]

On 8/5/19 10:59 AM, Sascha Hauer wrote:
> On Wed, Jul 31, 2019 at 12:21:42PM +0200, Ahmad Fatoum wrote:
>> Prior behavior was to wrongly report all bytes written if enqueueing wasn't
>> possible at the time. Instead we should either return 0 or an error code if
>> users need to retry. write(2) returns 0 in such cases. Follow suit.
>>
>> As no current users run puts in a loop, this has no effect for now.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> Changes in v2:
>> 	New commit.
>> ---
>>  common/ratp/ratp.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c
>> index 9aea1786d684..8ac7dc98b6f8 100644
>> --- a/common/ratp/ratp.c
>> +++ b/common/ratp/ratp.c
>> @@ -267,7 +267,7 @@ static int ratp_console_puts(struct console_device *cdev, const char *s)
>>  	len = strlen(s);
>>  
>>  	if (ratp_busy(&ctx->ratp))
>> -		return len;
>> +		return 0;
> 
> I'm not sure if this return value is ever used for something useful,
> not sure how relevant this is. ratp_busy() however returns true when
> it's called from inside the ratp code. This is necessary so that we
> don't get stuck in an endless loop. If we start returning 0 for
> "no characters sent" how should code evaluating this return value
> react? Retrying it until all characters are sent obviously is not an
> option.
> 
> I think the current behaviour of just returning 'len' is correct.

I see. I will drop this patch then.

> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/3] console: don't count newlines twice in bytes written

[Ahmad Fatoum]

On 8/5/19 11:11 AM, Sascha Hauer wrote:
> On Wed, Jul 31, 2019 at 12:21:41PM +0200, Ahmad Fatoum wrote:
>> Both the PBL and simple console only return number of input bytes, not
>> number of bytes actually written out. These differ, because each LF is
>> converted to CRLF pairs.
>>
>> The behavior of not counting actual written out characters is more sensible,
>> because otherwise callers interested in finding out if all bytes have been
>> written (e.g. to avoid incomplete writes with ratp) would need to keep count
>> of all line feeds in the string.
>> Therefore change the normal console to behave like its less featureful
>> brethren.
> 
> According to "man puts" puts() returns a non negative number on success.
> I can't find a place where it's claimed that we have to return the
> number of characters.

console_puts in common/console_simple.c and pbl/console.c already return the
number of characters. Same for all the console_device puts members in the
drivers.

> I also can't find a place where the return value
> of any puts like function in barebox is ever evaluated. Given that it
> might make more sense to just return zero in the absense of an error.

I too think there isn't, but it would be less surprising if common/console.c
would behave like other puts's in barebox.

> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 3/3] console: fix out-of-bounds read in dputc(/dev/*, ...)

[Ahmad Fatoum]

Hello Sascha,

On 7/31/19 12:21 PM, Ahmad Fatoum wrote:
> Trying to output a single character via
> 	echo -a /dev/serial0-1
> currently results in garbage output after the newline, because console.c's
> fops_write discards the buffer length and passes the buffer to
> (struct cdev)::puts which only handles NUL-terminated strings.
> 
> Fix this by amending (struct cdev)::puts with a new nbytes parameter,
> which is correctly propagated.

Can this one be merged? It doesn;t depend on the other two patches.

> 
> Fixes: b4f55fcf35 ("console: expose consoles in devfs")
> Cc: Bastian Krause <bst@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> Changes in v2:
> 	- fixed commit hash in Fixes:, by Bastian
> 	- fixed possible incomplete writes in __console_puts
> 	- rebased on top of two potential bug fixes
> ---
>  common/console.c            | 14 +++++++-------
>  common/ratp/ratp.c          | 10 ++++------
>  drivers/serial/efi-stdio.c  |  5 +++--
>  drivers/serial/serial_efi.c |  5 +++--
>  fs/pstore/platform.c        |  7 ++++---
>  include/console.h           |  2 +-
>  6 files changed, 22 insertions(+), 21 deletions(-)
> 
> diff --git a/common/console.c b/common/console.c
> index 52162df23b04..ce4122fa76f1 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -253,19 +253,19 @@ static void console_set_stdoutpath(struct console_device *cdev)
>  	free(str);
>  }
>  
> -static int __console_puts(struct console_device *cdev, const char *s)
> +static int __console_puts(struct console_device *cdev, const char *s,
> +			  size_t nbytes)
>  {
> -	int n = 0;
> +	size_t i;
>  
> -	while (*s) {
> +	for (i = 0; i < nbytes; i++) {
>  		if (*s == '\n')
>  			cdev->putc(cdev, '\r');
>  
>  		cdev->putc(cdev, *s);
> -		n++;
>  		s++;
>  	}
> -	return n;
> +	return i;
>  }
>  
>  static int fops_open(struct cdev *cdev, unsigned long flags)
> @@ -297,7 +297,7 @@ static ssize_t fops_write(struct cdev* dev, const void* buf, size_t count,
>  {
>  	struct console_device *priv = dev->priv;
>  
> -	priv->puts(priv, buf);
> +	priv->puts(priv, buf, count);
>  
>  	return 0;
>  }
> @@ -544,7 +544,7 @@ int console_puts(unsigned int ch, const char *str)
>  	if (initialized == CONSOLE_INIT_FULL) {
>  		for_each_console(cdev) {
>  			if (cdev->f_active & ch) {
> -				n = cdev->puts(cdev, str);
> +				n = cdev->puts(cdev, str, strlen(str));
>  			}
>  		}
>  		return n;
> diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c
> index 8ac7dc98b6f8..8f989515e485 100644
> --- a/common/ratp/ratp.c
> +++ b/common/ratp/ratp.c
> @@ -259,19 +259,17 @@ static int ratp_console_tstc(struct console_device *cdev)
>  	return kfifo_len(ctx->console_recv_fifo) ? 1 : 0;
>  }
>  
> -static int ratp_console_puts(struct console_device *cdev, const char *s)
> +static int ratp_console_puts(struct console_device *cdev, const char *s,
> +			     size_t nbytes)
>  {
>  	struct ratp_ctx *ctx = container_of(cdev, struct ratp_ctx, ratp_console);
> -	int len = 0;
> -
> -	len = strlen(s);
>  
>  	if (ratp_busy(&ctx->ratp))
>  		return 0;
>  
> -	kfifo_put(ctx->console_transmit_fifo, s, len);
> +	kfifo_put(ctx->console_transmit_fifo, s, nbytes);
>  
> -	return len;
> +	return nbytes;
>  }
>  
>  static void ratp_console_putc(struct console_device *cdev, char c)
> diff --git a/drivers/serial/efi-stdio.c b/drivers/serial/efi-stdio.c
> index 0703f727e766..2ca89fa4f861 100644
> --- a/drivers/serial/efi-stdio.c
> +++ b/drivers/serial/efi-stdio.c
> @@ -243,12 +243,13 @@ static int efi_process_key(struct efi_console_priv *priv, const char *inp)
>  	return 1;
>  }
>  
> -static int efi_console_puts(struct console_device *cdev, const char *s)
> +static int efi_console_puts(struct console_device *cdev, const char *s,
> +			    size_t nbytes)
>  {
>  	struct efi_console_priv *priv = to_efi(cdev);
>  	int n = 0;
>  
> -	while (*s) {
> +	while (nbytes--) {
>  		if (*s == 27) {
>  			priv->efi_console_buffer[n] = 0;
>  			priv->out->output_string(priv->out,
> diff --git a/drivers/serial/serial_efi.c b/drivers/serial/serial_efi.c
> index f0a2b22c2bc2..667d51f622ec 100644
> --- a/drivers/serial/serial_efi.c
> +++ b/drivers/serial/serial_efi.c
> @@ -130,13 +130,14 @@ static void efi_serial_putc(struct console_device *cdev, char c)
>  	serial->write(serial, &buffersize, &c);
>  }
>  
> -static int efi_serial_puts(struct console_device *cdev, const char *s)
> +static int efi_serial_puts(struct console_device *cdev, const char *s,
> +			   size_t nbytes)
>  {
>  	struct efi_serial_port *uart = to_efi_serial_port(cdev);
>  	struct efi_serial_io_protocol *serial = uart->serial;
>  	uint32_t control;
>  	efi_status_t efiret;
> -	unsigned long buffersize = strlen(s) * sizeof(char);
> +	unsigned long buffersize = nbytes;
>  
>  	do {
>  		efiret = serial->getcontrol(serial, &control);
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 755363c30999..601bfca6b025 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -67,10 +67,11 @@ static void pstore_console_write(const char *s, unsigned c)
>  	}
>  }
>  
> -static int pstore_console_puts(struct console_device *cdev, const char *s)
> +static int pstore_console_puts(struct console_device *cdev, const char *s,
> +			       size_t nbytes)
>  {
> -	pstore_console_write(s, strlen(s));
> -	return strlen(s);
> +	pstore_console_write(s, nbytes);
> +	return nbytes;
>  }
>  
>  static void pstore_console_putc(struct console_device *cdev, char c)
> diff --git a/include/console.h b/include/console.h
> index 673921331db7..1d86a584a7f2 100644
> --- a/include/console.h
> +++ b/include/console.h
> @@ -42,7 +42,7 @@ struct console_device {
>  
>  	int (*tstc)(struct console_device *cdev);
>  	void (*putc)(struct console_device *cdev, char c);
> -	int (*puts)(struct console_device *cdev, const char *s);
> +	int (*puts)(struct console_device *cdev, const char *s, size_t nbytes);
>  	int  (*getc)(struct console_device *cdev);
>  	int (*setbrg)(struct console_device *cdev, int baudrate);
>  	void (*flush)(struct console_device *cdev);
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/3] console: don't count newlines twice in bytes written

[Ahmad Fatoum]

Hello,

On 8/22/19 9:04 AM, Ahmad Fatoum wrote:
> 
> 
> On 8/5/19 11:11 AM, Sascha Hauer wrote:
>> On Wed, Jul 31, 2019 at 12:21:41PM +0200, Ahmad Fatoum wrote:
>>> Both the PBL and simple console only return number of input bytes, not
>>> number of bytes actually written out. These differ, because each LF is
>>> converted to CRLF pairs.
>>>
>>> The behavior of not counting actual written out characters is more sensible,
>>> because otherwise callers interested in finding out if all bytes have been
>>> written (e.g. to avoid incomplete writes with ratp) would need to keep count
>>> of all line feeds in the string.
>>> Therefore change the normal console to behave like its less featureful
>>> brethren.
>>
>> According to "man puts" puts() returns a non negative number on success.
>> I can't find a place where it's claimed that we have to return the
>> number of characters.
> 
> console_puts in common/console_simple.c and pbl/console.c already return the
> number of characters. Same for all the console_device puts members in the
> drivers.
> 
>> I also can't find a place where the return value
>> of any puts like function in barebox is ever evaluated. Given that it
>> might make more sense to just return zero in the absense of an error.
> 
> I too think there isn't, but it would be less surprising if common/console.c
> would behave like other puts's in barebox.

Sorry, I mixed it up. Ye, it's arguable whether they should return number
of input bytes written or zero. But no other puts in barebox counts new lines
twice, so I think the patch has its merit, even if a follow up patch in future
has puts functions return 0 instead.

> 
>>
>> Sascha
>>
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 3/3] console: fix out-of-bounds read in dputc(/dev/*, ...)

[Sascha Hauer]

On Thu, Aug 22, 2019 at 09:06:09AM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 7/31/19 12:21 PM, Ahmad Fatoum wrote:
> > Trying to output a single character via
> > 	echo -a /dev/serial0-1
> > currently results in garbage output after the newline, because console.c's
> > fops_write discards the buffer length and passes the buffer to
> > (struct cdev)::puts which only handles NUL-terminated strings.
> > 
> > Fix this by amending (struct cdev)::puts with a new nbytes parameter,
> > which is correctly propagated.
> 
> Can this one be merged? It doesn;t depend on the other two patches.

Ok. Can you rebase it? Without the others it doesn't seem to apply.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 3/3] console: fix out-of-bounds read in dputc(/dev/*, ...)

[Ahmad Fatoum]

Hello Sascha,

On 8/23/19 9:07 AM, Sascha Hauer wrote:
> On Thu, Aug 22, 2019 at 09:06:09AM +0200, Ahmad Fatoum wrote:
>> Hello Sascha,
>>
>> On 7/31/19 12:21 PM, Ahmad Fatoum wrote:
>>> Trying to output a single character via
>>> 	echo -a /dev/serial0-1
>>> currently results in garbage output after the newline, because console.c's
>>> fops_write discards the buffer length and passes the buffer to
>>> (struct cdev)::puts which only handles NUL-terminated strings.
>>>
>>> Fix this by amending (struct cdev)::puts with a new nbytes parameter,
>>> which is correctly propagated.
>>
>> Can this one be merged? It doesn;t depend on the other two patches.
> 
> Ok. Can you rebase it? Without the others it doesn't seem to apply.

done. I dropped the RATP one, but squashed the other.

> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox