[pacman-dev] [PATCH] package.c, fix incorrect buffersize

[Dave Reisner]

On Sun, Nov 01, 2015 at 01:32:59AM +0100, Rikard Falkeborn wrote:
> Correct title_suffix_len to be the actual number of elements in
> the string (including the NUL-terminator) instead of the size
> of a pointer.
> 
> Note that wmemcpy blindly copies the number of wide characters it is told
> to copy (no check for NUL-terminating character), so this previously copied
> data outside of title_suffix.
> 
> Signed-off-by: Rikard Falkeborn <rikard.falkeborn at gmail.com>
> ---

Just a thought -- instead of dealing with calculating fixed sized
buffers, why don't we just calculate the max width needed, and then use
printf to do the actual formatting? It prevents two whole classes of
problems: buffer overflows and potential string truncation.

The patch is somewhat insidious as it requires modifying list_display,
string_display, etc., but I've already got a mostly working POC already.

>  src/pacman/package.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/pacman/package.c b/src/pacman/package.c
> index dbd23f5..59f4327 100644
> --- a/src/pacman/package.c
> +++ b/src/pacman/package.c
> @@ -79,6 +79,8 @@ enum {
>   * potential growth.
>   */
>  #define TITLE_MAXLEN 50
> +#define TITLE_SUFFIX L" :"
> +#define TITLE_SUFFIX_LEN (ARRAYSIZE(TITLE_SUFFIX))
>  
>  static char titles[_T_MAX][TITLE_MAXLEN * sizeof(wchar_t)];
>  
> @@ -90,9 +92,7 @@ static void make_aligned_titles(void)
>  {
>  	unsigned int i;
>  	size_t max = 0;
> -	static const wchar_t *title_suffix = L" :";
> -	static const size_t title_suffix_len = sizeof(title_suffix);
> -	wchar_t wbuf[ARRAYSIZE(titles)][TITLE_MAXLEN + title_suffix_len];
> +	wchar_t wbuf[ARRAYSIZE(titles)][TITLE_MAXLEN + TITLE_SUFFIX_LEN];
>  	size_t wlen[ARRAYSIZE(wbuf)];
>  	char *buf[ARRAYSIZE(wbuf)];
>  	buf[T_ARCHITECTURE] = _("Architecture");
> @@ -133,7 +133,7 @@ static void make_aligned_titles(void)
>  
>  	for(i = 0; i < ARRAYSIZE(wbuf); i++) {
>  		wmemset(wbuf[i] + wlen[i], L' ', max - wlen[i]);
> -		wmemcpy(wbuf[i] + max, title_suffix, title_suffix_len);
> +		wmemcpy(wbuf[i] + max, TITLE_SUFFIX, TITLE_SUFFIX_LEN);
>  		wcstombs(titles[i], wbuf[i], sizeof(wbuf[i]));
>  	}
>  }
> -- 
> 2.6.2