Re: [pacman-dev] [PATCH] pacman/callback: fix buffer over-read

[László Várady]

Hi,

Thanks for the review!


> Ok, but maybe we should remove the now redundant null termination after
> the if block.
>
>
I believe the '\0' character after the if block is not completely
redundant, it
terminates the stripped package name which can be shorter than the original
string. `len` is modified according to this operation.

I'm moving the mentioned line inside the if statement.

--
László Várady

Re: [pacman-dev] [PATCH] pacman/callback: fix buffer over-read

[Dave Reisner]

On Sat, Aug 03, 2019 at 01:27:35AM +0200, László Várady wrote:
> Commit 11ab9aa9f5f0f3873df89c73e8715b82f485bd9b replaced a strcpy() call
> with memcpy(), without copying the terminating null character.
> 
> Since fname is allocated with malloc(), subsequent strstr() calls will
> overrun the buffer's boundary.
> 
> Signed-off-by: László Várady 
> ---
>  src/pacman/callback.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/pacman/callback.c b/src/pacman/callback.c
> index 22865614..a4c637ce 100644
> --- a/src/pacman/callback.c
> +++ b/src/pacman/callback.c
> @@ -765,7 +765,7 @@ void cb_dl_progress(const char *filename, off_t 
> file_xfered, off_t file_total)
>  
>   len = strlen(filename);
>   fname = malloc(len + 1);
> - memcpy(fname, filename, len);
> + memcpy(fname, filename, len + 1);

Ok, but maybe we should remove the now redundant null termination after
the if block.

>   /* strip package or DB extension for cleaner look */
>   if((p = strstr(fname, ".pkg")) || (p = strstr(fname, ".db")) || (p = 
> strstr(fname, ".files"))) {
>   /* tack on a .sig suffix for signatures */
> -- 
> 2.22.0

Re: [pacman-dev] [PATCH] pacman/callback: fix buffer over-read

[Andrew Gregory]

On 08/03/19 at 01:27am, László Várady wrote:
> Commit 11ab9aa9f5f0f3873df89c73e8715b82f485bd9b replaced a strcpy() call
> with memcpy(), without copying the terminating null character.
> 
> Since fname is allocated with malloc(), subsequent strstr() calls will
> overrun the buffer's boundary.
> 
> Signed-off-by: László Várady 

ACK.

[pacman-dev] [PATCH] pacman/callback: fix buffer over-read

[László Várady]

Commit 11ab9aa9f5f0f3873df89c73e8715b82f485bd9b replaced a strcpy() call
with memcpy(), without copying the terminating null character.

Since fname is allocated with malloc(), subsequent strstr() calls will
overrun the buffer's boundary.

Signed-off-by: László Várady 
---
 src/pacman/callback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/pacman/callback.c b/src/pacman/callback.c
index 22865614..a4c637ce 100644
--- a/src/pacman/callback.c
+++ b/src/pacman/callback.c
@@ -765,7 +765,7 @@ void cb_dl_progress(const char *filename, off_t 
file_xfered, off_t file_total)
 
len = strlen(filename);
fname = malloc(len + 1);
-   memcpy(fname, filename, len);
+   memcpy(fname, filename, len + 1);
/* strip package or DB extension for cleaner look */
if((p = strstr(fname, ".pkg")) || (p = strstr(fname, ".db")) || (p = 
strstr(fname, ".files"))) {
/* tack on a .sig suffix for signatures */
-- 
2.22.0