@matheusbsbs / matheus-bagliano / commits / e9ba678175

enter_repo: convert fixed-size buffers to strbufs

We use two PATH_MAX-sized buffers to represent the repo path, and must make sure not to overflow them. We do take care to check the lengths, but the logic is rather hard to follow, as we use several magic numbers (e.g., "PATH_MAX - 10"). And in fact you _can_ overflow the buffer if you have a ".git" file with an extremely long path in it. By switching to strbufs, these problems all go away. We do, however, retain the check that the initial input we get is no larger than PATH_MAX. This function is an entry point for untrusted repo names from the network, and it's a good idea to keep a sanity check (both to avoid allocating arbitrary amounts of memory, and also as a layer of defense against any downstream users of the names). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Sep 24, 2015 at 17:07 UTC e9ba678175da28607d57043e1363c6252880dd7f
1 file changed +29 -28
path.c
+29 -28
@@ -391,8 +391,8 @@ return_null:
391 */
392 const char *enter_repo(const char *path, int strict)
393 {
394 - static char used_path[PATH_MAX];
395 - static char validated_path[PATH_MAX];
394 + static struct strbuf validated_path = STRBUF_INIT;
395 + static struct strbuf used_path = STRBUF_INIT;
396
397 if (!path)
398 return NULL;
@@ -407,46 +407,47 @@ const char *enter_repo(const char *path, int strict)
407 while ((1 < len) && (path[len-1] == '/'))
408 len--;
409
410 + /*
411 + * We can handle arbitrary-sized buffers, but this remains as a
412 + * sanity check on untrusted input.
413 + */
414 if (PATH_MAX <= len)
415 return NULL;
412 - strncpy(used_path, path, len); used_path[len] = 0 ;
413 - strcpy(validated_path, used_path);
416
415 - if (used_path[0] == '~') {
416 - char *newpath = expand_user_path(used_path);
417 - if (!newpath || (PATH_MAX - 10 < strlen(newpath))) {
418 - free(newpath);
417 + strbuf_reset(&used_path);
418 + strbuf_reset(&validated_path);
419 + strbuf_add(&used_path, path, len);
420 + strbuf_add(&validated_path, path, len);
421 +
422 + if (used_path.buf[0] == '~') {
423 + char *newpath = expand_user_path(used_path.buf);
424 + if (!newpath)
425 return NULL;
420 - }
421 - /*
422 - * Copy back into the static buffer. A pity
423 - * since newpath was not bounded, but other
424 - * branches of the if are limited by PATH_MAX
425 - * anyway.
426 - */
427 - strcpy(used_path, newpath); free(newpath);
426 + strbuf_attach(&used_path, newpath, strlen(newpath),
427 + strlen(newpath));
428 }
429 - else if (PATH_MAX - 10 < len)
430 - return NULL;
431 - len = strlen(used_path);
429 for (i = 0; suffix[i]; i++) {
430 struct stat st;
434 - strcpy(used_path + len, suffix[i]);
435 - if (!stat(used_path, &st) &&
431 + size_t baselen = used_path.len;
432 + strbuf_addstr(&used_path, suffix[i]);
433 + if (!stat(used_path.buf, &st) &&
434 (S_ISREG(st.st_mode) ||
437 - (S_ISDIR(st.st_mode) && is_git_directory(used_path)))) {
438 - strcat(validated_path, suffix[i]);
435 + (S_ISDIR(st.st_mode) && is_git_directory(used_path.buf)))) {
436 + strbuf_addstr(&validated_path, suffix[i]);
437 break;
438 }
439 + strbuf_setlen(&used_path, baselen);
440 }
441 if (!suffix[i])
442 return NULL;
444 - gitfile = read_gitfile(used_path) ;
445 - if (gitfile)
446 - strcpy(used_path, gitfile);
447 - if (chdir(used_path))
443 + gitfile = read_gitfile(used_path.buf) ;
444 + if (gitfile) {
445 + strbuf_reset(&used_path);
446 + strbuf_addstr(&used_path, gitfile);
447 + }
448 + if (chdir(used_path.buf))
449 return NULL;
449 - path = validated_path;
450 + path = validated_path.buf;
451 }
452 else if (chdir(path))
453 return NULL;