[PATCH] Fixes for undefined behavior in util_tdb

[Andreas Schneider]

On Thursday, 29 November 2018 21:53:15 CET Gary Lockyer wrote:
> I'm not sure about the 2nd patch, comments in line.

I've added the panic, a pipeline with the smb_panic is running here:

https://gitlab.com/samba-team/devel/samba/pipelines/38463754


Thanks for the review,


	Andreas


> From 7c7cadda8ab039df4c5eedcf01dea445f737e16b Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Thu, 22 Nov 2018 13:33:11 +0100
> Subject: [PATCH 2/3] s3:lib: Fix undefined behavior in tdb_pack()
> 
> util_tdb.c:98:5: runtime error: null pointer passed as argument 2, which
> is declared to never be null
> 
> This means the second argument of memcpy() can't be NULL.
> 
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
>  source3/lib/util_tdb.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/source3/lib/util_tdb.c b/source3/lib/util_tdb.c
> index 4f2450c5773..182b27e6376 100644
> --- a/source3/lib/util_tdb.c
> +++ b/source3/lib/util_tdb.c
> @@ -76,14 +76,12 @@ static size_t tdb_pack_va(uint8_t *buf, int bufsize,
> const char *fmt, va_list ap
>  				SIVAL(buf, 0, d);
>  			break;
>  		case 'P': /* null-terminated string */
> -			s = va_arg(ap,char *);
> -			w = strlen(s);
> -			len = w + 1;
> -			if (bufsize && bufsize >= len)
> -				memcpy(buf, s, len);
> -			break;
>  		case 'f': /* null-terminated string */
>  			s = va_arg(ap,char *);
> +			if (s == NULL) {
> +
> 				len = 1;
> I think this should be an smb_panic, otherwise a random byte will be
> silently written to the buffer. While it could write a zero byte, then
> we'd be converting a NULL pointer to a zero length string.
> 
> +				break;
> +			}
>  			w = strlen(s);
>  			len = w + 1;
>  			if (bufsize && bufsize >= len)
> @@ -95,7 +93,9 @@ static size_t tdb_pack_va(uint8_t *buf, int bufsize,
> const char *fmt, va_list ap
>  			len = 4+i;
>  			if (bufsize && bufsize >= len) {
>  				SIVAL(buf, 0, i);
> -				memcpy(buf+4, s, i);
> +				if (s != NULL) {
> +					memcpy(buf+4, s, i);
> +				}
>  			}
>  			break;
> 
>  		default:
> > Hi,
> > 
> > see attached patchset. A CI pipeline is here:
> > 
> > https://gitlab.com/samba-team/devel/samba/pipelines/38025491
> > 
> > 
> > Please review and comment. Push if OK.
> > 
> > 
> > Thanks,
> > 
> > 	Andreas
> 
> Ngā mihi
> Gary


-- 
Andreas Schneider                      asn at samba.org
Samba Team                             www.samba.org
GPG-ID:     8DFF53E18F2ABC8D8F3C92237EE0FC4DCC014E3D