[pacman-dev] [PATCH] libalpm: Do a sanity check before manipulating final DB URL

[David Macek]

On 16. 4. 2015 21:36, Florian Pritz wrote:
> On 13.04.2015 21:33, David Macek wrote:
>> +			/* if we downloaded a DB, we want the .sig from the same server */
>> +			if(final_db_url != NULL) {
>>  				/* print final_db_url into a buffer (leave space for .sig) */
>>  				len = strlen(final_db_url) + 5;
>>  			} else {
>> -				/* print server + filename into a buffer (leave space for .sig) */
>> +				/* print server + filename into a buffer (leave space for .db.sig) */
>>  				len = strlen(server) + strlen(db->treename) + 9;
> 
> Comment changed, but not the code?

The comment was copied when the code was split into the two cases and the parenthesized part apparently wasn't correctly updated.

> If you know where that + 9 comes from
> it might be a good idea to replace it with strlen("whatever") which the
> compile should optimize out later,

I didn't know gcc could do that, thanks for the tip.

> but writing it this way makes the
> whole thing a lot clearer.

First off, isn't this change better suited for a separate patch? If yes -- in order to speed up the potential merge -- the comment change can be removed from this patch and we can talk about the lengths in the context of another patch.

The 9 there is for "/", ".db.sig", and the null terminator. Would this be a good way of re-writing the line?

strlen(server) + strlen("/") + strlen(db->treename) + strlen(".db.sig") + 1;

Warning: The following ideas might be too academic for this situation.

Since the strlen's are just duplicating the information contained in the format string, wouldn't it be better to use snsprintf to count the buffer length? I imagine the flow would be like this:

format = "...";
len = snprintf(NULL, 0, format, ...) + 1;
MALLOC(payload.fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
snprintf(payload.fileurl, len, format, ...);

Only I don't know how to make it work nice with the conditional code. Consider this:

format = final_db_url != NULL ? "%1$s.sig" : "%2$s/%3$s.db.sig";
len = snprintf(NULL, 0, format, final_db_url, server, db->treename) + 1;
MALLOC(payload.fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
snprintf(payload.fileurl, len, format, final_db_url, server, db->treename);

This code is almost DRY, but various sources say that skipping parameters in *printf is bad, so that doesn't work either.

Ideas?

-- 
David Macek

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4234 bytes
Desc: S/MIME Cryptographic Signature
URL: <https://lists.archlinux.org/pipermail/pacman-dev/attachments/20150416/148d5443/attachment.p7s>