Re: [PATCH v2 2/3] path: use size_t for dir_prefix length
K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
> The strlen() function returns a size_t. Storing this in a standard
> signed int is a bad practice that invites overflow vulnerabilities if
> paths get absurdly long.
"a standard signed int" -> "an int variable". There is no "nonstandard signed int" anyway ;-)
If we were doing malloc(len) using length truncated due to integer wraparound and then strcpy() the whole string, it would make us write beyond the end of the allocation, but in this case, the worst thing that can happen is that we stop comparing prematurely, which may make us declare that buf is a path inside the directory dir when it isn't. The two callers of this function do not use this miscalculated len to carry out what they do, so there is no other damage. It indeed would be computing a wrong result, but "overflow vulnerabilities" is a slight exaggeration in the context of this patch, I think.
"overflow vulnerabilities" -> "bugs due to integer wraparound".
Show 23 quoted lines
> Switch the variable to size_t. This is safe to do because 'len' is
> strictly used as an argument to strncmp() (which expects size_t) and
> as a positive array index, involving no signed arithmetic that could
> rely on negative values.
>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---
> path.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/path.c b/path.c
> index f613d8bbd1..56be5e1726 100644
> --- a/path.c
> +++ b/path.c
> @@ -58,7 +58,7 @@ static void strbuf_cleanup_path(struct strbuf *sb)
>
> static int dir_prefix(const char *buf, const char *dir)
> {
> - int len = strlen(dir);
> + size_t len = strlen(dir);
> return !strncmp(buf, dir, len) &&
> (is_dir_sep(buf[len]) || buf[len] == '\0');
> }
K Jayatheerth
