[pacman-dev] [PATCH] Size handling was changed in fgets() functions
Dan McGee
dpmcgee at gmail.com
Wed Oct 14 23:16:03 EDT 2009
On Tue, Oct 13, 2009 at 10:27 PM, Laszlo Papp <djszapi2 at gmail.com> wrote:
> Pacman's fgets function in the API used hardcoded numbers to identify the size.
> This is not good practice, so replace them with sizeof handling.
>
> Signed-off-by: Laszlo Papp <djszapi at archlinux.us>
> ---
So I like the idea, then I actually looked closer at this code (code
involved in the patch, not the patch). Why in one case do we allocate
a buffer of 513 and always be sure to fgets() 512, while everywhere
else our buffer is the same size as the fgets read? That sounds like a
problem to me... (the one in src/pacman/util.c is easy to break the
str* functions called as you can't guarantee null termination).
In addition, having "sizeof(line)-1" (and not even spacing consistent
with the coding style!) is a bit stupid, and much longer than "512"
was. Maybe a local variable or define or something, and at the least,
use +1 in the allocation so we don't need -1 x 18 times.
And out of scope, but grep() is completely broken if the term you are
looking for falls on the boundry of the 1024 character buffer.
You'd never guess a simple patch like this could garner so much attention. :)
> lib/libalpm/be_files.c | 48 ++++++++++++++++++++++++------------------------
> lib/libalpm/trans.c | 2 +-
> src/pacman/util.c | 2 +-
> 3 files changed, 26 insertions(+), 26 deletions(-)
>
> diff --git a/lib/libalpm/be_files.c b/lib/libalpm/be_files.c
> index 53bbda1..fa007ec 100644
> --- a/lib/libalpm/be_files.c
> +++ b/lib/libalpm/be_files.c
> @@ -418,7 +418,7 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq)
> info->name, inforeq);
>
> /* clear out 'line', to be certain - and to make valgrind happy */
> - memset(line, 0, 513);
> + memset(line, 0, sizeof(line));
>
> pkgpath = get_pkgpath(db, info);
>
> @@ -442,7 +442,7 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq)
> }
> _alpm_strtrim(line);
> if(strcmp(line, "%NAME%") == 0) {
> - if(fgets(line, 512, fp) == NULL) {
> + if(fgets(line, sizeof(line)-1, fp) == NULL) {
> goto error;
> }
> if(strcmp(_alpm_strtrim(line), info->name) != 0) {
> @@ -450,7 +450,7 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq)
> "mismatch on package %s\n"), db->treename, info->name);
> }
> } else if(strcmp(line, "%VERSION%") == 0) {
> - if(fgets(line, 512, fp) == NULL) {
> + if(fgets(line, sizeof(line)-1, fp) == NULL) {
> goto error;
> }
> if(strcmp(_alpm_strtrim(line), info->version) != 0) {
> @@ -458,39 +458,39 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq)
> "mismatch on package %s\n"), db->treename, info->name);
> }
> } else if(strcmp(line, "%FILENAME%") == 0) {
> - if(fgets(line, 512, fp) == NULL) {
> + if(fgets(line, sizeof(line)-1, fp) == NULL) {
> goto error;
> }
> STRDUP(info->filename, _alpm_strtrim(line), goto error);
> } else if(strcmp(line, "%DESC%") == 0) {
> - if(fgets(line, 512, fp) == NULL) {
> + if(fgets(line, sizeof(line)-1, fp) == NULL) {
> goto error;
> }
> STRDUP(info->desc, _alpm_strtrim(line), goto error);
> } else if(strcmp(line, "%GROUPS%") == 0) {
> - while(fgets(line, 512, fp) && strlen(_alpm_strtrim(line))) {
> + while(fgets(line, sizeof(line)-1, fp) && strlen(_alpm_strtrim(line))) {
> char *linedup;
> STRDUP(linedup, _alpm_strtrim(line), goto error);
> info->groups = alpm_list_add(info->groups, linedup);
> }
> } else if(strcmp(line, "%URL%") == 0) {
> - if(fgets(line, 512, fp) == NULL) {
> + if(fgets(line, sizeof(line)-1, fp) == NULL) {
> goto error;
> }
> STRDUP(info->url, _alpm_strtrim(line), goto error);
> } else if(strcmp(line, "%LICENSE%") == 0) {
> - while(fgets(line, 512, fp) && strlen(_alpm_strtrim(line))) {
> + while(fgets(line, sizeof(line)-1, fp) && strlen(_alpm_strtrim(line))) {
> char *linedup;
> STRDUP(linedup, _alpm_strtrim(line), goto error);
> info->licenses = alpm_list_add(info->licenses, linedup);
> }
> } else if(strcmp(line, "%ARCH%") == 0) {
> - if(fgets(line, 512, fp) == NULL) {
> + if(fgets(line, sizeof(line)-1, fp) == NULL) {
> goto error;
> }
> STRDUP(info->arch, _alpm_strtrim(line), goto error);
> } else if(strcmp(line, "%BUILDDATE%") == 0) {
> - if(fgets(line, 512, fp) == NULL) {
> + if(fgets(line, sizeof(line)-1, fp) == NULL) {
> goto error;
> }
> _alpm_strtrim(line);
> @@ -506,7 +506,7 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq)
> info->builddate = atol(line);
> }
> } else if(strcmp(line, "%INSTALLDATE%") == 0) {
> - if(fgets(line, 512, fp) == NULL) {
> + if(fgets(line, sizeof(line)-1, fp) == NULL) {
> goto error;
> }
> _alpm_strtrim(line);
> @@ -522,12 +522,12 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq)
> info->installdate = atol(line);
> }
> } else if(strcmp(line, "%PACKAGER%") == 0) {
> - if(fgets(line, 512, fp) == NULL) {
> + if(fgets(line, sizeof(line)-1, fp) == NULL) {
> goto error;
> }
> STRDUP(info->packager, _alpm_strtrim(line), goto error);
> } else if(strcmp(line, "%REASON%") == 0) {
> - if(fgets(line, 512, fp) == NULL) {
> + if(fgets(line, sizeof(line)-1, fp) == NULL) {
> goto error;
> }
> info->reason = (pmpkgreason_t)atol(_alpm_strtrim(line));
> @@ -537,7 +537,7 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq)
> * is currently only used in sync databases, and SIZE is
> * only used in local databases.
> */
> - if(fgets(line, 512, fp) == NULL) {
> + if(fgets(line, sizeof(line)-1, fp) == NULL) {
> goto error;
> }
> info->size = atol(_alpm_strtrim(line));
> @@ -548,19 +548,19 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq)
> } else if(strcmp(line, "%ISIZE%") == 0) {
> /* ISIZE (installed size) tag only appears in sync repositories,
> * not the local one. */
> - if(fgets(line, 512, fp) == NULL) {
> + if(fgets(line, sizeof(line)-1, fp) == NULL) {
> goto error;
> }
> info->isize = atol(_alpm_strtrim(line));
> } else if(strcmp(line, "%MD5SUM%") == 0) {
> /* MD5SUM tag only appears in sync repositories,
> * not the local one. */
> - if(fgets(line, 512, fp) == NULL) {
> + if(fgets(line, sizeof(line)-1, fp) == NULL) {
> goto error;
> }
> STRDUP(info->md5sum, _alpm_strtrim(line), goto error);
> } else if(strcmp(line, "%REPLACES%") == 0) {
> - while(fgets(line, 512, fp) && strlen(_alpm_strtrim(line))) {
> + while(fgets(line, sizeof(line)-1, fp) && strlen(_alpm_strtrim(line))) {
> char *linedup;
> STRDUP(linedup, _alpm_strtrim(line), goto error);
> info->replaces = alpm_list_add(info->replaces, linedup);
> @@ -583,13 +583,13 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq)
> while(fgets(line, 256, fp)) {
> _alpm_strtrim(line);
> if(strcmp(line, "%FILES%") == 0) {
> - while(fgets(line, 512, fp) && strlen(_alpm_strtrim(line))) {
> + while(fgets(line, sizeof(line)-1, fp) && strlen(_alpm_strtrim(line))) {
> char *linedup;
> STRDUP(linedup, _alpm_strtrim(line), goto error);
> info->files = alpm_list_add(info->files, linedup);
> }
> } else if(strcmp(line, "%BACKUP%") == 0) {
> - while(fgets(line, 512, fp) && strlen(_alpm_strtrim(line))) {
> + while(fgets(line, sizeof(line)-1, fp) && strlen(_alpm_strtrim(line))) {
> char *linedup;
> STRDUP(linedup, _alpm_strtrim(line), goto error);
> info->backup = alpm_list_add(info->backup, linedup);
> @@ -611,24 +611,24 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq)
> fgets(line, 255, fp);
> _alpm_strtrim(line);
> if(strcmp(line, "%DEPENDS%") == 0) {
> - while(fgets(line, 512, fp) && strlen(_alpm_strtrim(line))) {
> + while(fgets(line, sizeof(line)-1, fp) && strlen(_alpm_strtrim(line))) {
> pmdepend_t *dep = _alpm_splitdep(_alpm_strtrim(line));
> info->depends = alpm_list_add(info->depends, dep);
> }
> } else if(strcmp(line, "%OPTDEPENDS%") == 0) {
> - while(fgets(line, 512, fp) && strlen(_alpm_strtrim(line))) {
> + while(fgets(line, sizeof(line)-1, fp) && strlen(_alpm_strtrim(line))) {
> char *linedup;
> STRDUP(linedup, _alpm_strtrim(line), goto error);
> info->optdepends = alpm_list_add(info->optdepends, linedup);
> }
> } else if(strcmp(line, "%CONFLICTS%") == 0) {
> - while(fgets(line, 512, fp) && strlen(_alpm_strtrim(line))) {
> + while(fgets(line, sizeof(line)-1, fp) && strlen(_alpm_strtrim(line))) {
> char *linedup;
> STRDUP(linedup, _alpm_strtrim(line), goto error);
> info->conflicts = alpm_list_add(info->conflicts, linedup);
> }
> } else if(strcmp(line, "%PROVIDES%") == 0) {
> - while(fgets(line, 512, fp) && strlen(_alpm_strtrim(line))) {
> + while(fgets(line, sizeof(line)-1, fp) && strlen(_alpm_strtrim(line))) {
> char *linedup;
> STRDUP(linedup, _alpm_strtrim(line), goto error);
> info->provides = alpm_list_add(info->provides, linedup);
> @@ -647,7 +647,7 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq)
> fgets(line, 255, fp);
> _alpm_strtrim(line);
> if(strcmp(line, "%DELTAS%") == 0) {
> - while(fgets(line, 512, fp) && strlen(_alpm_strtrim(line))) {
> + while(fgets(line, sizeof(line)-1, fp) && strlen(_alpm_strtrim(line))) {
> pmdelta_t *delta = _alpm_delta_parse(line);
> if(delta) {
> info->deltas = alpm_list_add(info->deltas, delta);
> diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c
> index 65bd464..e4ecde3 100644
> --- a/lib/libalpm/trans.c
> +++ b/lib/libalpm/trans.c
> @@ -320,7 +320,7 @@ static int grep(const char *fn, const char *needle)
> }
> while(!feof(fp)) {
> char line[1024];
> - fgets(line, 1024, fp);
> + fgets(line, sizeof(line), fp);
> if(feof(fp)) {
> continue;
> }
> diff --git a/src/pacman/util.c b/src/pacman/util.c
> index 0e5e7f5..d57550a 100644
> --- a/src/pacman/util.c
> +++ b/src/pacman/util.c
> @@ -602,7 +602,7 @@ static int question(short preset, char *fmt, va_list args)
> return(preset);
> }
>
> - if(fgets(response, 32, stdin)) {
> + if(fgets(response, sizeof(response), stdin)) {
> strtrim(response);
> if(strlen(response) == 0) {
> return(preset);
> --
> 1.6.4.4
>
>
>
More information about the pacman-dev
mailing list