[pacman-dev] [PATCH] Size handling was changed in fgets() functions
./src/pacman/util.c:
- response size handling was changed in fgets() function, because
it's easier to handle the source modification after a size modification
./lib/libalp/be_files.c:
- line size handling was changed in fgets() function, because
it's easier to handle the source code after a size modification
./lib/libalpm/trans.c:
- line size handling was changed in fgets() function, because
it's easier to handle the source code after a size modification
Signed-off-by: Laszlo Papp
Laszlo Papp wrote:
./src/pacman/util.c: - response size handling was changed in fgets() function, because it's easier to handle the source modification after a size modification ./lib/libalp/be_files.c: - line size handling was changed in fgets() function, because it's easier to handle the source code after a size modification ./lib/libalpm/trans.c: - line size handling was changed in fgets() function, because it's easier to handle the source code after a size modification
Your commit messages really do not tell me much... and in this case are quite repetitive.
<snip> diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index c99f596..c182510 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; }
This highlights my concerns. We are removing a known size and instead recalculating it. What is the advantage of this? Allan
On Sun, Sep 20, 2009 at 11:21:19PM +1000, Allan McRae wrote:
Laszlo Papp wrote:
<snip> diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index c99f596..c182510 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; }
This highlights my concerns. We are removing a known size and instead recalculating it. What is the advantage of this?
If the size of line is ever changed, every single instance of the hardcoded number has to be changed which can be error prone. Buffer overruns are a real possibility. While this gains you nothing if line is *never* changed, it is best practice to not hardcode any value which is used in multiple places. -- Jeff My other computer is an abacus.
On Sep 20, 2009, at 3:21 PM, Allan McRae wrote:
<snip> diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index c99f596..c182510 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; }
This highlights my concerns. We are removing a known size and instead recalculating it. What is the advantage of this?
It's a compile-time calculation, so there's really no disadvantage. It's just safe programming, as Jeff pointed out. The commit message isn't very clear though.
Pacman's fgets function in the API used hardcoded numbers to identify the size.
This is not good practoce, so replace them with sizeof handling.
Signed-off-by: Laszlo Papp
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
On Tue, Oct 13, 2009 at 10:27 PM, Laszlo Papp
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
---
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
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
On Sat, Oct 17, 2009 at 4:54 PM, Laszlo Papp
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
---
Doesn't apply to git HEAD... dmcgee@galway ~/projects/pacman (master) $ git am -3 -s < /tmp/pacman-sizeof.patch Applying: Size handling was changed in fgets() functions Using index info to reconstruct a base tree... error: patch failed: lib/libalpm/trans.c:320 error: lib/libalpm/trans.c: patch does not apply error: patch failed: src/pacman/util.c:580 error: src/pacman/util.c: patch does not apply Did you hand edit your patch? It does not apply to blobs recorded in its index. Cannot fall back to three-way merge. Patch failed at 0001 Size handling was changed in fgets() functions When you have resolved this problem run "git am -3 --resolved". If you would prefer to skip this patch, instead run "git am -3 --skip". To restore the original branch and stop patching run "git am -3 --abort".
lib/libalpm/be_files.c | 49 ++++++++++++++++++++++++----------------------- lib/libalpm/trans.c | 3 +- src/pacman/util.c | 3 +- 3 files changed, 29 insertions(+), 26 deletions(-)
diff --git a/lib/libalpm/be_files.c b/lib/libalpm/be_files.c index 53bbda1..ffbaa8d 100644 --- a/lib/libalpm/be_files.c +++ b/lib/libalpm/be_files.c @@ -387,6 +387,7 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq) FILE *fp = NULL; char path[PATH_MAX]; char line[513]; + int sline = sizeof(line)-1; char *pkgpath = NULL;
ALPM_LOG_FUNC; @@ -418,7 +419,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, sline+1);
pkgpath = get_pkgpath(db, info);
@@ -442,7 +443,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, sline, fp) == NULL) { goto error; } if(strcmp(_alpm_strtrim(line), info->name) != 0) { @@ -450,7 +451,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, sline, fp) == NULL) { goto error; } if(strcmp(_alpm_strtrim(line), info->version) != 0) { @@ -458,39 +459,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, sline, 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, sline, 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, sline, 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, sline, 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, sline, 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, sline, 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, sline, fp) == NULL) { goto error; } _alpm_strtrim(line); @@ -506,7 +507,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, sline, fp) == NULL) { goto error; } _alpm_strtrim(line); @@ -522,12 +523,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, sline, 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, sline, fp) == NULL) { goto error; } info->reason = (pmpkgreason_t)atol(_alpm_strtrim(line)); @@ -537,7 +538,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, sline, fp) == NULL) { goto error; } info->size = atol(_alpm_strtrim(line)); @@ -548,19 +549,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, sline, 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, sline, 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, sline, fp) && strlen(_alpm_strtrim(line))) { char *linedup; STRDUP(linedup, _alpm_strtrim(line), goto error); info->replaces = alpm_list_add(info->replaces, linedup); @@ -583,13 +584,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, sline, 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, sline, fp) && strlen(_alpm_strtrim(line))) { char *linedup; STRDUP(linedup, _alpm_strtrim(line), goto error); info->backup = alpm_list_add(info->backup, linedup); @@ -611,24 +612,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, sline, 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, sline, 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, sline, 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, sline, fp) && strlen(_alpm_strtrim(line))) { char *linedup; STRDUP(linedup, _alpm_strtrim(line), goto error); info->provides = alpm_list_add(info->provides, linedup); @@ -647,7 +648,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, sline, 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..aea71db 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -320,7 +320,8 @@ static int grep(const char *fn, const char *needle) } while(!feof(fp)) { char line[1025]; - fgets(line, 1024, fp); + int sline = sizeof(line)-1; + fgets(line, sline, fp); if(feof(fp)) { continue; } diff --git a/src/pacman/util.c b/src/pacman/util.c index 0e5e7f5..1143bef 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -580,6 +580,7 @@ void display_optdepends(pmpkg_t *pkg) static int question(short preset, char *fmt, va_list args) { char response[33]; + int sresponse = sizeof(response)-1; FILE *stream;
if(config->noconfirm) { @@ -602,7 +603,7 @@ static int question(short preset, char *fmt, va_list args) return(preset); }
- if(fgets(response, 32, stdin)) { + if(fgets(response, sresponse, stdin)) { strtrim(response); if(strlen(response) == 0) { return(preset); -- 1.6.4.4
On Mon, Oct 19, 2009 at 3:35 AM, Dan McGee
On Sat, Oct 17, 2009 at 4:54 PM, Laszlo Papp
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
--- Doesn't apply to git HEAD...
dmcgee@galway ~/projects/pacman (master) $ git am -3 -s < /tmp/pacman-sizeof.patch Applying: Size handling was changed in fgets() functions Using index info to reconstruct a base tree... error: patch failed: lib/libalpm/trans.c:320 error: lib/libalpm/trans.c: patch does not apply error: patch failed: src/pacman/util.c:580 error: src/pacman/util.c: patch does not apply Did you hand edit your patch? It does not apply to blobs recorded in its index. Cannot fall back to three-way merge. Patch failed at 0001 Size handling was changed in fgets() functions When you have resolved this problem run "git am -3 --resolved". If you would prefer to skip this patch, instead run "git am -3 --skip". To restore the original branch and stop patching run "git am -3 --abort".
I don't know what's the problem by you, but it works here fine with this procedure: git clone git://projects.archlinux.org/pacman.git pacman && cd pacman && git checkout -b working && git apply 0001-Size-handling-was-changed-in-fgets-functions.patch Could you check it ? I've attached the patch file. Best Regards, Laszlo Papp
On Sun, Oct 18, 2009 at 11:39 PM, Laszlo Papp
On Mon, Oct 19, 2009 at 3:35 AM, Dan McGee
wrote: On Sat, Oct 17, 2009 at 4:54 PM, Laszlo Papp
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
--- Doesn't apply to git HEAD...
dmcgee@galway ~/projects/pacman (master) $ git am -3 -s < /tmp/pacman-sizeof.patch Applying: Size handling was changed in fgets() functions Using index info to reconstruct a base tree... error: patch failed: lib/libalpm/trans.c:320 error: lib/libalpm/trans.c: patch does not apply error: patch failed: src/pacman/util.c:580 error: src/pacman/util.c: patch does not apply Did you hand edit your patch? It does not apply to blobs recorded in its index. Cannot fall back to three-way merge. Patch failed at 0001 Size handling was changed in fgets() functions When you have resolved this problem run "git am -3 --resolved". If you would prefer to skip this patch, instead run "git am -3 --skip". To restore the original branch and stop patching run "git am -3 --abort".
I don't know what's the problem by you, but it works here fine with this procedure:
git clone git://projects.archlinux.org/pacman.git pacman && cd pacman && git checkout -b working && git apply 0001-Size-handling-was-changed-in-fgets-functions.patch
Could you check it ? I've attached the patch file.
The "problem by me" is you just sent me two completely different patch
files, one of which was missing necessary spaces along with having
numerous other differences. This one does apply (so thanks), but look
at how different it is- not even close to the same.
-Dan
dmcgee@galway ~/projects/pacman (master)
$ diff -u /tmp/pacman-sizeof.patch
/tmp/0001-Size-handling-was-changed-in-fgets-functions.patch
--- /tmp/pacman-sizeof.patch 2009-10-19 07:39:35.770019492 -0500
+++ /tmp/0001-Size-handling-was-changed-in-fgets-functions.patch 2009-10-18
23:35:13.000000000 -0500
@@ -1,6 +1,7 @@
-From: Laszlo Papp
participants (6)
-
Allan McRae
-
Dan McGee
-
Jeff
-
Laszlo Papp
-
Laszlo Papp
-
Sebastian Nowicki