[PATCH 1/1] csv: detect end of csv file correctly

[Peter Jones]

On Sun, Aug 29, 2021 at 09:12:47PM +0200, Heinrich Schuchardt wrote:
> Shim produces random failures like
> 
>     sbat.c:46:parse_sbat_section() row->n_columns:0 SBAT_SECTION_COLUMNS:6
>     Could not parse .sbat section data: Invalid Parameter
> 
> Adding debug statements shows output like
> 
>     csv.c:60:parse_csv_data() sbat[0x0]:
>     sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
>     shim,1,UEFI shim,shim,1,https://github.com/rhboot/shim
> 
>     csv.c:16:parse_csv_line() line:
>     sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
>     csv.c:35:parse_csv_line() n_columns 6
>     csv.c:16:parse_csv_line() line:
>     shim,1,UEFI shim,shim,1,https://github.com/rhboot/shim
>     csv.c:35:parse_csv_line() n_columns 6
>     csv.c:16:parse_csv_line() line:
> 
>     csv.c:35:parse_csv_line() n_columns 0
>     csv.c:16:parse_csv_line() line:
> 
>     csv.c:35:parse_csv_line() n_columns 0
>     csv.c:16:parse_csv_line() line:

I agree with your patch and with the summary below, but the above could
use some improvement.  Under what conditions does it fail?  What makes
you categorize the failures as "random"?  What do we need to have in the
.sbat section to make it fail this way?  What could we add to test-csv.c
or test-sbat.c to make it reproduce the and thus verify that it is in
fact fixed and stays that way?

> In parse_csv_data() variable 'line' is a pointer with a value between
> the values of 'data' and 'data_end'.
> 
> There is no reason to check that that it is non-zero after assigning it
> from 'data' as we already check 'data'.
> 
> Instead after each line we must check that we have not reached the end of
> the file marked by a '\0' character.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> ---
>  csv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/csv.c b/csv.c
> index f6b37f1..b716632 100644
> --- a/csv.c
> +++ b/csv.c
> @@ -63,10 +63,10 @@ parse_csv_data(char *data, char *data_end, size_t n_columns, list_t *list)
>  
>  	max = (uintptr_t)end - (uintptr_t)line + (end > line ? 1 : 0);
>  
> -	if (line && is_utf8_bom(line, max))
> +	if (is_utf8_bom(line, max))
>  		line += UTF8_BOM_SIZE;
>  
> -	while (line && line <= data_end) {
> +	while (line <= data_end && *line) {
>  		size_t entrysz = sizeof(char *) * n_columns + sizeof(struct csv_row);
>  		struct csv_row *entry;
>  		size_t m_columns = n_columns;
> -- 
> 2.30.2
> 

-- 
        Peter