[PATCH] Fixes for undefined behavior in util_tdb

[Gary Lockyer]

Looks good to me, RB+ and pushed to autobuild

Ngā mihi

Gary

On 30/11/18 23:52, Andreas Schneider wrote:
> 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
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20181203/407f40d2/signature.sig>