diff mbox series

[v3,1/2,delta_handler] BUG: Fix read source loop in create_zckindex

Message ID 20240315161108.218579-1-mege.mathieu@gmail.com
State Accepted
Headers show
Series [v3,1/2,delta_handler] BUG: Fix read source loop in create_zckindex | expand

Commit Message

Mathieu MEGE March 15, 2024, 4:11 p.m. UTC 
-  deal with read() return error code
-  add missing total bytes increment

Signed-off-by: Mathieu Mege <mege.mathieu@gmail.com>
---
Changes for v2:
- more readable way to malloc buffer
- fix strerror param
Changes for v3:
- make the patch more focused on bug

 handlers/delta_handler.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

Comments

Pierre-Jean Texier March 15, 2024, 4:39 p.m. UTC | #1 
Hi Mathieu,

Le 15/03/2024 à 17:11, Mathieu Mege a écrit :
> -  deal with read() return error code
> -  add missing total bytes increment
> 
> Signed-off-by: Mathieu Mege <mege.mathieu@gmail.com>

Acked-by: Pierre-Jean Texier <texier.pj2@gmail.com>

Thanks !
--
Pierre-Jean Texier
Stefano Babic March 17, 2024, 4:13 p.m. UTC | #2 
On 15.03.24 17:11, Mathieu Mege wrote:
> -  deal with read() return error code
> -  add missing total bytes increment
>
> Signed-off-by: Mathieu Mege <mege.mathieu@gmail.com>
> ---
> Changes for v2:
> - more readable way to malloc buffer
> - fix strerror param
> Changes for v3:
> - make the patch more focused on bug
>
>   handlers/delta_handler.c | 39 ++++++++++++++++++++++++++++++---------
>   1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/handlers/delta_handler.c b/handlers/delta_handler.c
> index 295cd5f..8f0c97e 100644
> --- a/handlers/delta_handler.c
> +++ b/handlers/delta_handler.c
> @@ -45,6 +45,7 @@
>   #include "swupdate_image.h"
>
>   #define DEFAULT_MAX_RANGES	150	/* Apache has default = 200 */
> +#define BUFF_SIZE		16384
>
>   const char *handlername = "delta";
>   void delta_handler(void);
> @@ -470,34 +471,54 @@ static void zck_log_toswupdate(const char *function, zck_log_type lt,
>
>   /*
>    * Create a zck Index from a file
> + *
> + * If maxbytes has been set, it acts as a limit for the input data.
> + * If not (i.e. maxbytes==0), all the file/dev available data is used.
>    */
>   static bool create_zckindex(zckCtx *zck, int fd, size_t maxbytes)
>   {
> -	const size_t bufsize = 16384;
> -	char *buf = malloc(bufsize);
> -	ssize_t n;
> -	int ret;
> +	size_t bufsize = BUFF_SIZE;
> +	char *buf = malloc(BUFF_SIZE);
> +	ssize_t n = 0;
> +	size_t count = 0;
> +	bool rstatus = true;
>
>   	if (!buf) {
>   		ERROR("OOM creating temporary buffer");
>   		return false;
>   	}
>   	while ((n = read(fd, buf, bufsize)) > 0) {
> -		ret = zck_write(zck, buf, n);
> -		if (ret < 0) {
> +		if (zck_write(zck, buf, n) < 0) {
>   			ERROR("ZCK returns %s", zck_get_error(zck));
>   			free(buf);
>   			return false;
>   		}
> -		if (maxbytes && n > maxbytes)
> -			break;
> +
> +		if(maxbytes) {
> +			/* Keep count only if maxbytes has been set and it's significant*/
> +			count += n;
> +
> +			/* Stop if limit is reached*/
> +			if (count >= maxbytes)
> +				break;
> +
> +			/* Be sure read up to maxbytes limit next time */
> +			if (BUFF_SIZE > (maxbytes - count))
> +				bufsize = maxbytes - count;
> +		}
>   	}
>
>   	free(buf);
>
> -	return true;
> +	if(n < 0) {
> +		ERROR("Error occurred while reading data : %s", strerror(n));
> +		rstatus = false;
> +	}
> +
> +	return rstatus;
>   }
>
> +
>   /*
>    * Chunks must be retrieved from network, prepare an send
>    * a request for the downloader


Acked-by: Stefano Babic <stefano.babic@swupdate.org>

Best regards,
Stefano Babic
Stefano Babic March 21, 2024, 3:35 p.m. UTC | #3 
On 17.03.24 17:13, Stefano Babic wrote:
> 
> 
> On 15.03.24 17:11, Mathieu Mege wrote:
>> -  deal with read() return error code
>> -  add missing total bytes increment
>>
>> Signed-off-by: Mathieu Mege <mege.mathieu@gmail.com>
>> ---
>> Changes for v2:
>> - more readable way to malloc buffer
>> - fix strerror param
>> Changes for v3:
>> - make the patch more focused on bug
>>
>>   handlers/delta_handler.c | 39 ++++++++++++++++++++++++++++++---------
>>   1 file changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/handlers/delta_handler.c b/handlers/delta_handler.c
>> index 295cd5f..8f0c97e 100644
>> --- a/handlers/delta_handler.c
>> +++ b/handlers/delta_handler.c
>> @@ -45,6 +45,7 @@
>>   #include "swupdate_image.h"
>>
>>   #define DEFAULT_MAX_RANGES    150    /* Apache has default = 200 */
>> +#define BUFF_SIZE        16384
>>
>>   const char *handlername = "delta";
>>   void delta_handler(void);
>> @@ -470,34 +471,54 @@ static void zck_log_toswupdate(const char 
>> *function, zck_log_type lt,
>>
>>   /*
>>    * Create a zck Index from a file
>> + *
>> + * If maxbytes has been set, it acts as a limit for the input data.
>> + * If not (i.e. maxbytes==0), all the file/dev available data is used.
>>    */
>>   static bool create_zckindex(zckCtx *zck, int fd, size_t maxbytes)
>>   {
>> -    const size_t bufsize = 16384;
>> -    char *buf = malloc(bufsize);
>> -    ssize_t n;
>> -    int ret;
>> +    size_t bufsize = BUFF_SIZE;
>> +    char *buf = malloc(BUFF_SIZE);
>> +    ssize_t n = 0;
>> +    size_t count = 0;
>> +    bool rstatus = true;
>>
>>       if (!buf) {
>>           ERROR("OOM creating temporary buffer");
>>           return false;
>>       }
>>       while ((n = read(fd, buf, bufsize)) > 0) {
>> -        ret = zck_write(zck, buf, n);
>> -        if (ret < 0) {
>> +        if (zck_write(zck, buf, n) < 0) {
>>               ERROR("ZCK returns %s", zck_get_error(zck));
>>               free(buf);
>>               return false;
>>           }
>> -        if (maxbytes && n > maxbytes)
>> -            break;
>> +
>> +        if(maxbytes) {
>> +            /* Keep count only if maxbytes has been set and it's 
>> significant*/
>> +            count += n;
>> +
>> +            /* Stop if limit is reached*/
>> +            if (count >= maxbytes)
>> +                break;
>> +
>> +            /* Be sure read up to maxbytes limit next time */
>> +            if (BUFF_SIZE > (maxbytes - count))
>> +                bufsize = maxbytes - count;
>> +        }
>>       }
>>
>>       free(buf);
>>
>> -    return true;
>> +    if(n < 0) {
>> +        ERROR("Error occurred while reading data : %s", strerror(n));
>> +        rstatus = false;
>> +    }

Coverity reports that a negative value is passed, where no negative 
value is expected. Really strerror() expects an int, but surely we call 
strerror(EINVAL) and not strerror(-EINVAL).

I will drop these 4 lines, but you can post a new patch just for this.

Best regards,
Stefano

>> +
>> +    return rstatus;
>>   }
>>
>> +
>>   /*
>>    * Chunks must be retrieved from network, prepare an send
>>    * a request for the downloader
> 
> 
> Acked-by: Stefano Babic <stefano.babic@swupdate.org>
> 
> Best regards,
> Stefano Babic
>
Stefano Babic March 21, 2024, 3:38 p.m. UTC | #4 
On 21.03.24 16:35, Stefano Babic wrote:
> On 17.03.24 17:13, Stefano Babic wrote:
>>
>>
>> On 15.03.24 17:11, Mathieu Mege wrote:
>>> -  deal with read() return error code
>>> -  add missing total bytes increment
>>>
>>> Signed-off-by: Mathieu Mege <mege.mathieu@gmail.com>
>>> ---
>>> Changes for v2:
>>> - more readable way to malloc buffer
>>> - fix strerror param
>>> Changes for v3:
>>> - make the patch more focused on bug
>>>
>>>   handlers/delta_handler.c | 39 ++++++++++++++++++++++++++++++---------
>>>   1 file changed, 30 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/handlers/delta_handler.c b/handlers/delta_handler.c
>>> index 295cd5f..8f0c97e 100644
>>> --- a/handlers/delta_handler.c
>>> +++ b/handlers/delta_handler.c
>>> @@ -45,6 +45,7 @@
>>>   #include "swupdate_image.h"
>>>
>>>   #define DEFAULT_MAX_RANGES    150    /* Apache has default = 200 */
>>> +#define BUFF_SIZE        16384
>>>
>>>   const char *handlername = "delta";
>>>   void delta_handler(void);
>>> @@ -470,34 +471,54 @@ static void zck_log_toswupdate(const char
>>> *function, zck_log_type lt,
>>>
>>>   /*
>>>    * Create a zck Index from a file
>>> + *
>>> + * If maxbytes has been set, it acts as a limit for the input data.
>>> + * If not (i.e. maxbytes==0), all the file/dev available data is used.
>>>    */
>>>   static bool create_zckindex(zckCtx *zck, int fd, size_t maxbytes)
>>>   {
>>> -    const size_t bufsize = 16384;
>>> -    char *buf = malloc(bufsize);
>>> -    ssize_t n;
>>> -    int ret;
>>> +    size_t bufsize = BUFF_SIZE;
>>> +    char *buf = malloc(BUFF_SIZE);
>>> +    ssize_t n = 0;
>>> +    size_t count = 0;
>>> +    bool rstatus = true;
>>>
>>>       if (!buf) {
>>>           ERROR("OOM creating temporary buffer");
>>>           return false;
>>>       }
>>>       while ((n = read(fd, buf, bufsize)) > 0) {
>>> -        ret = zck_write(zck, buf, n);
>>> -        if (ret < 0) {
>>> +        if (zck_write(zck, buf, n) < 0) {
>>>               ERROR("ZCK returns %s", zck_get_error(zck));
>>>               free(buf);
>>>               return false;
>>>           }
>>> -        if (maxbytes && n > maxbytes)
>>> -            break;
>>> +
>>> +        if(maxbytes) {
>>> +            /* Keep count only if maxbytes has been set and it's
>>> significant*/
>>> +            count += n;
>>> +
>>> +            /* Stop if limit is reached*/
>>> +            if (count >= maxbytes)
>>> +                break;
>>> +
>>> +            /* Be sure read up to maxbytes limit next time */
>>> +            if (BUFF_SIZE > (maxbytes - count))
>>> +                bufsize = maxbytes - count;
>>> +        }
>>>       }
>>>
>>>       free(buf);
>>>
>>> -    return true;
>>> +    if(n < 0) {
>>> +        ERROR("Error occurred while reading data : %s", strerror(n));
>>> +        rstatus = false;
>>> +    }
>
> Coverity reports that a negative value is passed, where no negative
> value is expected. Really strerror() expects an int, but surely we call
> strerror(EINVAL) and not strerror(-EINVAL).
>
> I will drop these 4 lines, but you can post a new patch just for this.

Not exactly what I do: I just replace strerror(n) with strerror(errno)

Best regards,
Stefano

>
> Best regards,
> Stefano
>
>>> +
>>> +    return rstatus;
>>>   }
>>>
>>> +
>>>   /*
>>>    * Chunks must be retrieved from network, prepare an send
>>>    * a request for the downloader
>>
>>
>> Acked-by: Stefano Babic <stefano.babic@swupdate.org>
>>
>> Best regards,
>> Stefano Babic
>>
>
diff mbox series

Patch

diff --git a/handlers/delta_handler.c b/handlers/delta_handler.c
index 295cd5f..8f0c97e 100644
--- a/handlers/delta_handler.c
+++ b/handlers/delta_handler.c
@@ -45,6 +45,7 @@ 
 #include "swupdate_image.h"
 
 #define DEFAULT_MAX_RANGES	150	/* Apache has default = 200 */
+#define BUFF_SIZE		16384
 
 const char *handlername = "delta";
 void delta_handler(void);
@@ -470,34 +471,54 @@  static void zck_log_toswupdate(const char *function, zck_log_type lt,
 
 /*
  * Create a zck Index from a file
+ *
+ * If maxbytes has been set, it acts as a limit for the input data.
+ * If not (i.e. maxbytes==0), all the file/dev available data is used.
  */
 static bool create_zckindex(zckCtx *zck, int fd, size_t maxbytes)
 {
-	const size_t bufsize = 16384;
-	char *buf = malloc(bufsize);
-	ssize_t n;
-	int ret;
+	size_t bufsize = BUFF_SIZE;
+	char *buf = malloc(BUFF_SIZE);
+	ssize_t n = 0;
+	size_t count = 0;
+	bool rstatus = true;
 
 	if (!buf) {
 		ERROR("OOM creating temporary buffer");
 		return false;
 	}
 	while ((n = read(fd, buf, bufsize)) > 0) {
-		ret = zck_write(zck, buf, n);
-		if (ret < 0) {
+		if (zck_write(zck, buf, n) < 0) {
 			ERROR("ZCK returns %s", zck_get_error(zck));
 			free(buf);
 			return false;
 		}
-		if (maxbytes && n > maxbytes)
-			break;
+
+		if(maxbytes) {
+			/* Keep count only if maxbytes has been set and it's significant*/
+			count += n;
+
+			/* Stop if limit is reached*/
+			if (count >= maxbytes)
+				break;
+
+			/* Be sure read up to maxbytes limit next time */
+			if (BUFF_SIZE > (maxbytes - count))
+				bufsize = maxbytes - count;
+		}
 	}
 
 	free(buf);
 
-	return true;
+	if(n < 0) {
+		ERROR("Error occurred while reading data : %s", strerror(n));
+		rstatus = false;
+	}
+
+	return rstatus;
 }
 
+
 /*
  * Chunks must be retrieved from network, prepare an send
  * a request for the downloader