[pacman-dev] [PATCH 2/4] Change sprintf function using for snprintf

[Dan McGee]

On Sun, Oct 18, 2009 at 5:02 AM, Xavier <shiningxc at gmail.com> wrote:
> On Sun, Oct 18, 2009 at 6:03 AM, Laszlo Papp <djszapi2 at gmail.com> wrote:
>>        * Size examined str* function usage is a common coding practice, because it's
>>        more safer to avoid breakage while using str* functions.
>>
>> Signed-off-by: Laszlo Papp <djszapi at archlinux.us>
>> ---
>>  lib/libalpm/add.c      |    4 ++--
>>  lib/libalpm/be_files.c |   10 +++++-----
>>  lib/libalpm/conflict.c |    2 +-
>>  lib/libalpm/db.c       |    4 ++--
>>  lib/libalpm/trans.c    |    2 +-
>>  lib/libalpm/util.c     |   16 +++++++++-------
>>  src/pacman/sync.c      |    2 +-
>>  src/pacman/util.c      |    2 +-
>>  8 files changed, 22 insertions(+), 20 deletions(-)
>>
>> diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c
>> index ebcd6a5..209c38e 100644
>> --- a/lib/libalpm/add.c
>> +++ b/lib/libalpm/add.c
>> @@ -311,7 +311,7 @@ static int extract_single_file(struct archive *archive,
>>                        size_t backup_len = strlen(oldbackup) + 34;
>>                        MALLOC(backup, backup_len, RET_ERR(PM_ERR_MEMORY, -1));
>>
>> -                       sprintf(backup, "%s\t%s", oldbackup, hash_pkg);
>> +                       snprintf(backup, backup_len, "%s\t%s", oldbackup, hash_pkg);
>
> I don't think this provides us any extra safety, because we already
> compute exactly the size of the destination and allocate that memory.
>
> I think it makes more sense where we define a fixed array, like char
> path[PATH_MAX], and we then copy strings of different size to it :
> src/pacman/pacman.c:                            snprintf(path,
> PATH_MAX, "%s%s", alpm_option_get_root(), DBPATH + 1);
> src/pacman/pacman.c:                            snprintf(path,
> PATH_MAX, "%s%s", alpm_option_get_root(), LOGFILE + 1);

There is zero added safety to this. All of the patches that have been
coming in are relatively bogus. We've done strlen() in nearly all
cases anyway.

The "n" family of operations also silently truncates. This is probably
worse off for us, as it could lead to silent failures.

-Dan