[pacman-dev] Bug in libalpm: sizeof(off_t)

[Allan McRae]

On 12/12/13 02:44, Dave Reisner wrote:
> On Thu, Dec 12, 2013 at 02:14:19AM +1000, Allan McRae wrote:
>> On 12/12/13 00:24, Jeremy Heiner wrote:
>>> On Tue, Dec 10, 2013 at 11:58 PM, Allan McRae <allan at archlinux.org> wrote:
>>>> I am still looking for something cleaner than
>>>> the proposed #define/#undef approach which feels a bit hacky
>>>
>>> Here is a third (well, 4th, since "do nothing" is always on the table)
>>> option which lies between "just document" and "macro shenanigans"...
>>> Following the lead of 'alpm_capabilities', add:
>>>
>>
>> How about #5...
>>
>>
>> diff --git a/lib/libalpm/alpm.c b/lib/libalpm/alpm.c
>> index 878c38b..a41c07d 100644
>> --- a/lib/libalpm/alpm.c
>> +++ b/lib/libalpm/alpm.c
>> @@ -52,6 +52,12 @@ alpm_handle_t SYMEXPORT *alpm_initialize(const char
>> *root, const char *dbpath,
>>  	const char *lf = "db.lck";
>>  	size_t lockfilelen;
>>  	alpm_handle_t *myhandle = _alpm_handle_new();
>> +
>> +	/* calculate off_t size at runtime */
>> +	size_t off_t_size = ((char *)((off_t *)0 + 1) - (char *)(off_t *)0);
>> +
>> +	if(off_t_size != sizeof(off_t)) {
>> +		myerr = ALPM_ERR_OFF_T_SIZE;
>> +		goto cleanup;
>> +	}
> 
> I do not believe this actually works as intended. There's nothing "at
> runtime" about this, especially when optimizations are involved (even
> with -O1). Your off_t_size will be calculated at compile time and
> therefore the comparison that follows will never fail. The instructions
> generated for the off_t_size is effectively:
> 
>     mov    $0x8,%eax
> 
> Or, when LFS isn't enabled:
> 
>     mov   $0x4,%eax
> 

This version looks like it should survive optimization:

	off_t x;
	size_t off_t_size = ((char *)(&(x) + 1) - (char *)&(x));


> 
>>
>>  	if(myhandle == NULL) {
>>  		myerr = ALPM_ERR_MEMORY;
>> diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
>> index 5fa775c..f61e3f5 100644
>> --- a/lib/libalpm/alpm.h
>> +++ b/lib/libalpm/alpm.h
>> @@ -1333,6 +1333,7 @@ typedef enum _alpm_errno_t {
>>  	/* Misc */
>>  	ALPM_ERR_RETRIEVE,
>>  	ALPM_ERR_INVALID_REGEX,
>> +	ALPM_ERR_OFF_T_SIZE,
>>  	/* External library errors */
>>  	ALPM_ERR_LIBARCHIVE,
>>  	ALPM_ERR_LIBCURL,
>> diff --git a/lib/libalpm/error.c b/lib/libalpm/error.c
>> index 8622180..1aed1a1 100644
>> --- a/lib/libalpm/error.c
>> +++ b/lib/libalpm/error.c
>> @@ -145,6 +145,8 @@ const char SYMEXPORT *alpm_strerror(alpm_errno_t err)
>>  			return _("failed to retrieve some files");
>>  		case ALPM_ERR_INVALID_REGEX:
>>  			return _("invalid regular expression");
>> +		case ALPM_ERR_OFF_T_SIZE:
>> +			return _("incompatible off_t size");
>>  		/* Errors from external libraries- our own wrapper error */
>>  		case ALPM_ERR_LIBARCHIVE:
>>  			/* it would be nice to use archive_error_string() here, but that
>>
>>
>>
> 
> 
>