The supposed safety blanket of this function is better handled by
explicit length checking and usages of strlen() on known NULL-terminated
strings rather than hoping things fit in a buffer. We also have no need
to fully fill a PATH_MAX length variable with NULLs every time as long
as a single terminating byte is there. Remove usages of it by using
strcpy() or memcpy() as appropriate, after doing length checks via
strlen().

Signed-off-by: Dan McGee <dan@archlinux.org>
---
lib/libalpm/handle.c | 2 +-
src/pacman/query.c | 17 ++++++++++-------
src/pacman/util.c | 11 ++++-------
src/util/vercmp.c | 2 +-
4 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c
index 22f3fc8..b48d5b6 100644
--- a/lib/libalpm/handle.c
+++ b/lib/libalpm/handle.c
@@ -298,7 +298,7 @@ static char *canonicalize_path(const char *path) {
len += 1;
}
CALLOC(new_path, len + 1, sizeof(char), return NULL);
- strncpy(new_path, path, len);
+ strcpy(new_path, path);
new_path[len - 1] = '/';
return new_path;
}
diff --git a/src/pacman/query.c b/src/pacman/query.c
index 045dc7f..4fc4584 100644
--- a/src/pacman/query.c
+++ b/src/pacman/query.c
@@ -110,8 +110,7 @@ static int query_fileowner(alpm_list_t *targets)
int ret = 0;
char path[PATH_MAX];
const char *root;
- char *append;
- size_t max_length;
+ size_t rootlen;
alpm_list_t *t;
alpm_db_t *db_local;

@@ -125,9 +124,13 @@ static int query_fileowner(alpm_list_t *targets)
* once, then we can just overwrite whatever file was there on the previous
* iteration. */
root = alpm_option_get_root(config->handle);
- strncpy(path, root, PATH_MAX - 1);
- append = path + strlen(path);
- max_length = PATH_MAX - (append - path) - 1;
+ rootlen = strlen(root);
+ if(rootlen + 1 > PATH_MAX) {
+ /* we are in trouble here */
+ pm_fprintf(stderr, ALPM_LOG_ERROR, _("path too long: %s%s
"), root, "");
+ return 1;
+ }
+ strcpy(path, root);

db_local = alpm_option_get_localdb(config->handle);

@@ -208,11 +211,11 @@ static int query_fileowner(alpm_list_t *targets)
continue;
}

- if(strlen(pkgfile) > max_length) {
+ if(rootlen + 1 + strlen(pkgfile) > PATH_MAX) {
pm_fprintf(stderr, ALPM_LOG_ERROR, _("path too long: %s%s
"), root, pkgfile);
}
/* concatenate our file and the root path */
- strcpy(append, pkgfile);
+ strcpy(path + rootlen, pkgfile);

pdname = mdirname(path);
ppath = resolve_path(pdname);
diff --git a/src/pacman/util.c b/src/pacman/util.c
index 9ced7aa..203f927 100644
--- a/src/pacman/util.c
+++ b/src/pacman/util.c
@@ -361,22 +361,21 @@ char *strreplace(const char *str, const char *needle, const char *replace)
* x "size difference between replace and needle" */
newsz = strlen(str) + 1 +
alpm_list_count(list) * (replacesz - needlesz);
- newstr = malloc(newsz);
+ newstr = calloc(newsz, sizeof(char));
if(!newstr) {
return NULL;
}
- *newstr = '';

p = str;
newp = newstr;
for(i = list; i; i = alpm_list_next(i)) {
q = alpm_list_getdata(i);
- if(q > p){
+ if(q > p) {
/* add chars between this occurence and last occurence, if any */
- strncpy(newp, p, (size_t)(q - p));
+ memcpy(newp, p, (size_t)(q - p));
newp += q - p;
}
- strncpy(newp, replace, replacesz);
+ memcpy(newp, replace, replacesz);
newp += replacesz;
p = q + needlesz;
}
@@ -385,9 +384,7 @@ char *strreplace(const char *str, const char *needle, const char *replace)
if(*p) {
/* add the rest of 'p' */
strcpy(newp, p);
- newp += strlen(p);
}
- *newp = '';

return newstr;
}
diff --git a/src/util/vercmp.c b/src/util/vercmp.c
index 88cf49a..f4356fb 100644
--- a/src/util/vercmp.c
+++ b/src/util/vercmp.c
@@ -20,7 +20,7 @@

#include <stdlib.h>
#include <stdio.h> /* printf */
-#include <string.h> /* strncpy */
+#include <string.h>

#define BASENAME "vercmp"

--
1.7.6

On 06/07/11 05:20, Dan McGee wrote:

The supposed safety blanket of this function is better handled by
explicit length checking and usages of strlen() on known NULL-terminated
strings rather than hoping things fit in a buffer. We also have no need
to fully fill a PATH_MAX length variable with NULLs every time as long
as a single terminating byte is there. Remove usages of it by using
strcpy() or memcpy() as appropriate, after doing length checks via
strlen().

Signed-off-by: Dan McGee<dan@archlinux.org>


Ack-by: Allan

On Wed, Jul 6, 2011 at 3:20 AM, Dan McGee <dan@archlinux.org> wrote:
> The supposed safety blanket of this function is better handled by
> explicit length checking and usages of strlen() on known NULL-terminated
> strings rather than hoping things fit in a buffer. We also have no need
> to fully fill a PATH_MAX length variable with NULLs every time as long
> as a single terminating byte is there. Remove usages of it by using
> strcpy() or memcpy() as appropriate, after doing length checks via
> strlen().
>
> Signed-off-by: Dan McGee <dan@archlinux.org>
> ---
> *lib/libalpm/handle.c | * *2 +-
> *src/pacman/query.c * | * 17 ++++++++++-------
> *src/pacman/util.c * *| * 11 ++++-------
> *src/util/vercmp.c * *| * *2 +-
> *4 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c
> index 22f3fc8..b48d5b6 100644
> --- a/lib/libalpm/handle.c
> +++ b/lib/libalpm/handle.c
> @@ -125,9 +124,13 @@ static int query_fileowner(alpm_list_t *targets)
> * * * * * once, then we can just overwrite whatever file was there on the previous
> * * * * * iteration. */
> * * * *root = alpm_option_get_root(config->handle);
> - * * * strncpy(path, root, PATH_MAX - 1);
> - * * * append = path + strlen(path);
> - * * * max_length = PATH_MAX - (append - path) - 1;
> + * * * rootlen = strlen(root);
> + * * * if(rootlen + 1 > PATH_MAX) {
> + * * * * * * * /* we are in trouble here */
> + * * * * * * * pm_fprintf(stderr, ALPM_LOG_ERROR, _("path too long: %s%s
"), root, "");
> + * * * * * * * return 1;
> + * * * }
> + * * * strcpy(path, root);
>

Might be a bit late to the party, but there's potential for a bug to
creep in here. If anything is done with "path" before hand, the check
becomes incorrect. It should check strlen(path) + rootlen, and is done
later on:

> @@ -208,11 +211,11 @@ static int query_fileowner(alpm_list_t *targets)
> * * * * * * * * * * * * * * * * * * * *continue;
> * * * * * * * * * * * * * * * *}
>
> - * * * * * * * * * * * * * * * if(strlen(pkgfile) > max_length) {
> + * * * * * * * * * * * * * * * if(rootlen + 1 + strlen(pkgfile) > PATH_MAX) {
> * * * * * * * * * * * * * * * * * * * *pm_fprintf(stderr, ALPM_LOG_ERROR, _("path too long: %s%s
"), root, pkgfile);
> * * * * * * * * * * * * * * * *}
> * * * * * * * * * * * * * * * */* concatenate our file and the root path */
> - * * * * * * * * * * * * * * * strcpy(append, pkgfile);
> + * * * * * * * * * * * * * * * strcpy(path + rootlen, pkgfile);
>
> * * * * * * * * * * * * * * * *pdname = mdirname(path);
> * * * * * * * * * * * * * * * *ppath = resolve_path(pdname);