search

khanzf / opengit

OpenGit - A BSD licensed clone of Git for FreeBSD (under heavy development)
Other
50 stars 3 forks source link

Lots of potential buffer overflows. #3

Open ibara opened 5 years ago

ibara commented 5 years ago

gcc-8.2.0 on OpenBSD/amd64 flags a large number of potential buffer overflows. Complete build log:

egcc -O2 -pipe  -Wall -I/usr/local/include  -c -o ogit.o ogit.c
egcc -O2 -pipe  -Wall -I/usr/local/include  -c -o lib/ini.o lib/ini.c
lib/ini.c: In function 'config_parser':
lib/ini.c:61:23: warning: '/config' directive writing 7 bytes into a region of size between 1 and 1279 [-Wformat-overflow=]
  sprintf(ini_file, "%s/config", dotgitpath);
                       ^~~~~~~
lib/ini.c:61:2: note: 'sprintf' output between 8 and 1286 bytes into a destination of size 1279
  sprintf(ini_file, "%s/config", dotgitpath);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -c -o lib/index.o lib/index.c
egcc -O2 -pipe  -Wall -I/usr/local/include  -c -o lib/common.o lib/common.c
lib/common.c: In function 'git_repository_path':
lib/common.c:79:4: warning: 'strncpy' output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
    strncpy(dotgitpath, path, strlen(path));
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -c -o lib/pack.o lib/pack.c
lib/pack.c: In function 'pack_find_sha_offset':
lib/pack.c:392:19: warning: variable 'checksums' set but not used [-Wunused-but-set-variable]
  struct checksum *checksums;
                   ^~~~~~~~~
In file included from lib/pack.c:39:
At top level:
lib/pack.h:55:20: warning: 'object_name' defined but not used [-Wunused-variable]
 static const char *object_name[] = {
                    ^~~~~~~~~~~
lib/pack.c: In function 'pack_get_packfile_offset':
lib/pack.c:208:20: warning: '%s' directive writing up to 1278 bytes into a region of size 1024 [-Wformat-overflow=]
  sprintf(packdir, "%s/objects/pack", dotgitpath);
                    ^~                ~~~~~~~~~~
lib/pack.c:208:2: note: 'sprintf' output between 14 and 1292 bytes into a destination of size 1024
  sprintf(packdir, "%s/objects/pack", dotgitpath);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -c -o remote.o remote.c
remote.c: In function 'remote_remove':
remote.c:93:24: warning: '/.config.XXXXXX' directive writing 15 bytes into a region of size between 1 and 1279 [-Wformat-overflow=]
  sprintf(tmpconfig, "%s/.config.XXXXXX", dotgitpath);
                        ^~~~~~~~~~~~~~~
remote.c:93:2: note: 'sprintf' output between 16 and 1294 bytes into a destination of size 1279
  sprintf(tmpconfig, "%s/.config.XXXXXX", dotgitpath);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -c -o init.o init.c
init.c: In function 'bare_init':
init.c:61:17: warning: variable 'new_config' set but not used [-Wunused-but-set-variable]
  struct section new_config;
                 ^~~~~~~~~~
init.c: In function 'init_main':
init.c:140:10: warning: variable 'flags' set but not used [-Wunused-but-set-variable]
  uint8_t flags = 0;
          ^~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -c -o lib/zlib-handler.o lib/zlib-handler.c
egcc -O2 -pipe  -Wall -I/usr/local/include  -c -o hash-object.o hash-object.c
hash-object.c: In function 'hash_object_create_zlib':
hash-object.c:115:23: warning: '/objects/' directive writing 9 bytes into a region of size between 1 and 1279 [-Wformat-overflow=]
  sprintf(filepath, "%s/objects/%c%c", dotgitpath, checksum[0], checksum[1]);
                       ^~~~~~~~~
hash-object.c:115:2: note: 'sprintf' output between 12 and 1290 bytes into a destination of size 1279
  sprintf(filepath, "%s/objects/%c%c", dotgitpath, checksum[0], checksum[1]);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hash-object.c: In function 'hash_object_create_file':
hash-object.c:172:23: warning: '%s' directive writing up to 1278 bytes into a region of size 1024 [-Wformat-overflow=]
  sprintf(objectpath, "%s/objects/%c%c",
                       ^~
      dotgitpath, checksum[0], checksum[1]);
      ~~~~~~~~~~
hash-object.c:172:2: note: 'sprintf' output between 12 and 1290 bytes into a destination of size 1024
  sprintf(objectpath, "%s/objects/%c%c",
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      dotgitpath, checksum[0], checksum[1]);
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hash-object.c:178:23: warning: '%s' directive writing up to 1278 bytes into a region of size 1024 [-Wformat-overflow=]
  sprintf(objectpath, "%s/objects/%c%c/%s",
                       ^~
      dotgitpath, checksum[0], checksum[1], checksum+2);
      ~~~~~~~~~~
hash-object.c:178:2: note: 'sprintf' output 13 or more bytes (assuming 1291) into a destination of size 1024
  sprintf(objectpath, "%s/objects/%c%c/%s",
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      dotgitpath, checksum[0], checksum[1], checksum+2);
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -c -o update-index.o update-index.c
update-index.c: In function 'update_index_open_index':
update-index.c:62:22: warning: '%s' directive writing up to 1278 bytes into a region of size 1024 [-Wformat-overflow=]
  sprintf(indexpath, "%s/index", dotgitpath);
                      ^~         ~~~~~~~~~~
update-index.c:62:2: note: 'sprintf' output between 7 and 1285 bytes into a destination of size 1024
  sprintf(indexpath, "%s/index", dotgitpath);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -c -o cat-file.o cat-file.c
cat-file.c: In function 'cat_file_get_content_loose':
cat-file.c:154:23: warning: '%s' directive writing up to 1278 bytes into a region of size 1024 [-Wformat-overflow=]
  sprintf(objectpath, "%s/objects/%c%c/%s", dotgitpath, sha_str[0], sha_str[1], sha_str+2);
                       ^~                   ~~~~~~~~~~
cat-file.c:154:2: note: 'sprintf' output 13 or more bytes (assuming 1291) into a destination of size 1024
  sprintf(objectpath, "%s/objects/%c%c/%s", dotgitpath, sha_str[0], sha_str[1], sha_str+2);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cat-file.c: In function 'cat_file_main':
cat-file.c:320:4: warning: 'sha_str' may be used uninitialized in this function [-Wmaybe-uninitialized]
    cat_file_get_content(sha_str, flags);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -c -o log.o log.c
In file included from log.c:41:
lib/pack.h:55:20: warning: 'object_name' defined but not used [-Wunused-variable]
 static const char *object_name[] = {
                    ^~~~~~~~~~~
log.c: In function 'log_print_commit_headers':
log.c:81:4: warning: 'strncpy' specified bound 40 equals destination size [-Wstringop-truncation]
    strncpy(logarg->sha, token+7, 40);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
log.c: In function 'log_get_start_sha':
log.c:142:21: warning: '%s' directive writing up to 1278 bytes into a region of size 1024 [-Wformat-overflow=]
  sprintf(headfile, "%s/HEAD", dotgitpath);
                     ^~        ~~~~~~~~~~
log.c:142:2: note: 'sprintf' output between 6 and 1284 bytes into a destination of size 1024
  sprintf(headfile, "%s/HEAD", dotgitpath);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
log.c:155:21: warning: '%s' directive writing up to 1278 bytes into a region of size 1024 [-Wformat-overflow=]
   sprintf(refpath, "%s/%s", dotgitpath, ref);
                     ^~      ~~~~~~~~~~
log.c:155:3: note: 'sprintf' output between 2 and 2303 bytes into a destination of size 1024
   sprintf(refpath, "%s/%s", dotgitpath, ref);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
log.c: In function 'log_get_loose_object':
log.c:178:23: warning: '%s' directive writing up to 1278 bytes into a region of size 1024 [-Wformat-overflow=]
  sprintf(objectpath, "%s/objects/%c%c/%s",
                       ^~
      dotgitpath, logarg->sha[0], logarg->sha[1],
      ~~~~~~~~~~
log.c:178:2: note: 'sprintf' output 13 or more bytes (assuming 1291) into a destination of size 1024
  sprintf(objectpath, "%s/objects/%c%c/%s",
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      dotgitpath, logarg->sha[0], logarg->sha[1],
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      logarg->sha+2);
      ~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -c -o clone.o clone.c
In file included from clone.c:43:
lib/common.h:32: warning: "nitems" redefined
 #define nitems(x) (sizeof((x)) / sizeof((x)[0]))

In file included from clone.c:29:
/usr/include/sys/param.h:204: note: this is the location of the previous definition
 #define nitems(_a) (sizeof((_a)) / sizeof((_a)[0]))

clone.c: In function 'clone_pack_protocol_process':
clone.c:244:6: warning: variable 'check' set but not used [-Wunused-but-set-variable]
  int check;
      ^~~~~
clone.c: In function 'clone_http_get_sha':
clone.c:325:6: warning: variable 'content_length' set but not used [-Wunused-but-set-variable]
  int content_length;
      ^~~~~~~~~~~~~~
clone.c: In function 'clone_http_get_head':
clone.c:95:2: warning: 'strncpy' specified bound 41 equals destination size [-Wstringop-truncation]
  strncpy(smart_head.sha, position, 41);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -c -o index-pack.o index-pack.c
In file included from index-pack.c:42:
lib/common.h:32: warning: "nitems" redefined
 #define nitems(x) (sizeof((x)) / sizeof((x)[0]))

In file included from index-pack.c:28:
/usr/include/sys/param.h:204: note: this is the location of the previous definition
 #define nitems(_a) (sizeof((_a)) / sizeof((_a)[0]))

egcc  -o ogit    ogit.o lib/ini.o lib/index.o lib/common.o lib/pack.o remote.o init.o  lib/zlib-handler.o  hash-object.o update-index.o cat-file.o log.o clone.o index-pack.o -L/usr/local/lib -lfetch -lz
khanzf commented 5 years ago

This is exactly the sort of feedback I want, thank you very much! I'll try to get to these ASAP. Did you run anything in specific or just the Makefile as-is?

ibara commented 5 years ago

I had to modify the Makefile since fmake and omake have diverged significantly. But the build steps are (mostly) the same (I wrote the omake Makefile in a way that it doesn't do the depends generation).

I had to port libfetch too; wasn't particularly difficult.

khanzf commented 5 years ago

Finally found time, pardon the lengthy delay. Please review d0fcca8a0250ef5a8826137ba4bcf5fc149bf4ec to see if the issues have been resolved. Additionally, I recognize code-quality issues in general with the ini code, which I plan to refactor and clean up.

ibara commented 5 years ago

Here is a diff that fixes things. It also has some additional tweaks to cope with the differences between FreeBSD and OpenBSD. I hope the intent is clear. ogit-fix-buffers.txt

emaste commented 5 years ago

Also errors found by -fsanitize=address e.g.

% ./ogit log       
=================================================================
==36769==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61900000182f at pc 0x0000002c8c78 bp 0x7fffffff5590 sp 0x7fffffff4d30
READ of size 944 at 0x61900000182f thread T0
    #0 0x2c8c77 in __interceptor_strdup /usr/home/emaste/src/freebsd/contrib/compiler-rt/lib/asan/asan_interceptors.cc:445:5
    #1 0x2fc373 in log_print_commit_headers /usr/home/emaste/src/opengit/log.c:76:17
    #2 0x2fcbea in log_display_cb /usr/home/emaste/src/opengit/log.c:122:4
    #3 0x2f687f in deflate_caller /usr/home/emaste/src/opengit/lib/zlib-handler.c:153:8
    #4 0x2fdbfb in log_get_pack_object /usr/home/emaste/src/opengit/log.c:218:2
    #5 0x2fdf51 in log_display_commits /usr/home/emaste/src/opengit/log.c:235:42
    #6 0x2fe335 in log_main /usr/home/emaste/src/opengit/log.c:272:2
    #7 0x2e7847 in main /usr/home/emaste/src/opengit/ogit.c:81:2
    #8 0x24011f in _start /usr/home/emaste/src/freebsd/lib/csu/amd64/crt1.c:76:7

0x61900000182f is located 0 bytes to the right of 943-byte region [0x619000001480,0x61900000182f)
allocated by thread T0 here:
    #0 0x2d69ff in __interceptor_realloc /usr/home/emaste/src/freebsd/contrib/compiler-rt/lib/asan/asan_malloc_linux.cc:165:3
    #1 0x2fc9c3 in log_display_cb /usr/home/emaste/src/opengit/log.c:114:21
    #2 0x2f687f in deflate_caller /usr/home/emaste/src/opengit/lib/zlib-handler.c:153:8
    #3 0x2fdbfb in log_get_pack_object /usr/home/emaste/src/opengit/log.c:218:2
    #4 0x2fdf51 in log_display_commits /usr/home/emaste/src/opengit/log.c:235:42
    #5 0x2fe335 in log_main /usr/home/emaste/src/opengit/log.c:272:2
    #6 0x2e7847 in main /usr/home/emaste/src/opengit/ogit.c:81:2
    #7 0x24011f in _start /usr/home/emaste/src/freebsd/lib/csu/amd64/crt1.c:76:7
    #8 0x80032cfff  (<unknown module>)

SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/home/emaste/src/freebsd/contrib/compiler-rt/lib/asan/asan_interceptors.cc:445:5 in __interceptor_strdup
Shadow bytes around the buggy address:
  0x4c32000002b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x4c32000002c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x4c32000002d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x4c32000002e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x4c32000002f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x4c3200000300: 00 00 00 00 00[07]fa fa fa fa fa fa fa fa fa fa
  0x4c3200000310: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x4c3200000320: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x4c3200000330: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x4c3200000340: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x4c3200000350: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==36769==ABORTING
kevans91 commented 5 years ago

Mostly for my reference:

Compile for ASAN using -fsanitize=address Symbolize the reports with llvm-symbolizer; on FreeBSD, buildworld installworld with WITH_CLANG_EXTRAS=yes. Alternatively, set ASAN_SYMBOLIZER_PATH env var to an llvm-symbolizer binary.

khanzf commented 5 years ago

Thanks for the assistance everyone. @ibara provided the initial feedback, which I incorporated and I have since incorporated @kevans91's changes. Going forward, I am not familiar with the tooling that Kevin and @emaste suggested. I addeded "-fsanitize=address" to CFLAGS, but do not receive the output when running './ogit log'. Is this the result of the issue having been eliminated or am I using the tooling incorrectly?

ibara commented 5 years ago

gcc-8.3.0 still has a lot to say about things. Build log without -Wall:

egcc -O0 -pipe -I/usr/local/include  -c /home/brian/opengit/ogit.c
egcc -O0 -pipe -I/usr/local/include  -c /home/brian/opengit/lib/ini.c
/home/brian/opengit/lib/ini.c: In function 'ini_get_config':
/home/brian/opengit/lib/ini.c:170:2: warning: 'strncat' specified bound 12 equals source length [-Wstringop-overflow=]
  strncat(path, "/.git/config", 12);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O0 -pipe -I/usr/local/include  -c /home/brian/opengit/lib/index.c
egcc -O0 -pipe -I/usr/local/include  -c /home/brian/opengit/lib/common.c
egcc -O0 -pipe -I/usr/local/include  -c /home/brian/opengit/lib/pack.c
egcc -O0 -pipe -I/usr/local/include  -c /home/brian/opengit/remote.c
egcc -O0 -pipe -I/usr/local/include  -c /home/brian/opengit/init.c
/home/brian/opengit/init.c: In function 'init_dirinit':
/home/brian/opengit/init.c:106:4: warning: 'strncat' specified bound 1024 equals destination size [-Wstringop-overflow=]
    strncat(path, "/", PATH_MAX);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O0 -pipe -I/usr/local/include  -c /home/brian/opengit/lib/zlib-handler.c
egcc -O0 -pipe -I/usr/local/include  -c /home/brian/opengit/lib/buffering.c
egcc -O0 -pipe -I/usr/local/include  -c /home/brian/opengit/lib/loose.c
egcc -O0 -pipe -I/usr/local/include  -c /home/brian/opengit/hash-object.c
egcc -O0 -pipe -I/usr/local/include  -c /home/brian/opengit/update-index.c
egcc -O0 -pipe -I/usr/local/include  -c /home/brian/opengit/cat-file.c
egcc -O0 -pipe -I/usr/local/include  -c /home/brian/opengit/log.c
egcc -O0 -pipe -I/usr/local/include  -c /home/brian/opengit/clone.c
/home/brian/opengit/clone.c: In function 'clone_initial_config':
/home/brian/opengit/clone.c:548:2: warning: 'strncat' specified bound 12 equals source length [-Wstringop-overflow=]
  strncat(path, "/.git/config", 12);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O0 -pipe -I/usr/local/include  -c /home/brian/opengit/index-pack.c
egcc -lz -L/usr/local/lib -lfetch  -o ogit ogit.o ini.o index.o common.o pack.o remote.o init.o zlib-handler.o buffering.o loose.o hash-object.o update-index.o cat-file.o log.o clone.o index-pack.o

and with -Wall

egcc -O2 -pipe  -Wall -I/usr/local/include  -MD -MP  -c /home/brian/opengit/ogit.c
egcc -O2 -pipe  -Wall -I/usr/local/include  -MD -MP  -c /home/brian/opengit/lib/ini.c
/home/brian/opengit/lib/ini.c: In function 'ini_get_config':
/home/brian/opengit/lib/ini.c:170:2: warning: 'strncat' specified bound 12 equals source length [-Wstringop-overflow=]
  strncat(path, "/.git/config", 12);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/brian/opengit/lib/ini.c:169:2: warning: 'strncpy' output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
  strncpy(path, repodir, strlen(repodir));
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/brian/opengit/lib/ini.c: In function 'config_parser':
/home/brian/opengit/lib/ini.c:64:42: warning: '/config' directive output may be truncated writing 7 bytes into a region of size between 1 and 1024 [-Wformat-truncation=]
  snprintf(ini_file, sizeof(ini_file), "%s/config", dotgitpath);
                                          ^~~~~~~
/home/brian/opengit/lib/ini.c:64:2: note: 'snprintf' output between 8 and 1031 bytes into a destination of size 1024
  snprintf(ini_file, sizeof(ini_file), "%s/config", dotgitpath);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -MD -MP  -c /home/brian/opengit/lib/index.c
egcc -O2 -pipe  -Wall -I/usr/local/include  -MD -MP  -c /home/brian/opengit/lib/common.c
In file included from /home/brian/opengit/lib/common.c:39:
/home/brian/opengit/lib/pack.h:55:20: warning: 'object_name' defined but not used [-Wunused-variable]
 static const char *object_name[] = {
                    ^~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -MD -MP  -c /home/brian/opengit/lib/pack.c
/home/brian/opengit/lib/pack.c: In function 'pack_find_sha_offset':
/home/brian/opengit/lib/pack.c:651:19: warning: variable 'checksums' set but not used [-Wunused-but-set-variable]
  struct checksum *checksums;
                   ^~~~~~~~~
/home/brian/opengit/lib/pack.c: In function 'pack_get_packfile_offset':
/home/brian/opengit/lib/pack.c:455:40: warning: '/objects/pack' directive output may be truncated writing 13 bytes into a region of size between 1 and 1024 [-Wformat-truncation=]
  snprintf(packdir, sizeof(packdir), "%s/objects/pack", dotgitpath);
                                        ^~~~~~~~~~~~~
/home/brian/opengit/lib/pack.c:455:2: note: 'snprintf' output between 14 and 1037 bytes into a destination of size 1024
  snprintf(packdir, sizeof(packdir), "%s/objects/pack", dotgitpath);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/brian/opengit/lib/pack.c:465:36: warning: '/objects/pack/' directive output may be truncated writing 14 bytes into a region of size between 1 and 1024 [-Wformat-truncation=]
    snprintf(filename, PATH_MAX, "%s/objects/pack/%s", dotgitpath,
                                    ^~~~~~~~~~~~~~
/home/brian/opengit/lib/pack.c:465:4: note: 'snprintf' output between 15 and 1293 bytes into a destination of size 1024
    snprintf(filename, PATH_MAX, "%s/objects/pack/%s", dotgitpath,
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        dir->d_name);
        ~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -MD -MP  -c /home/brian/opengit/remote.c
/home/brian/opengit/remote.c: In function 'remote_remove':
/home/brian/opengit/remote.c:92:44: warning: '/.config.XXXXXX' directive output may be truncated writing 15 bytes into a region of size between 1 and 1024 [-Wformat-truncation=]
  snprintf(tmpconfig, sizeof(tmpconfig), "%s/.config.XXXXXX", dotgitpath);
                                            ^~~~~~~~~~~~~~~
/home/brian/opengit/remote.c:92:2: note: 'snprintf' output between 16 and 1039 bytes into a destination of size 1024
  snprintf(tmpconfig, sizeof(tmpconfig), "%s/.config.XXXXXX", dotgitpath);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -MD -MP  -c /home/brian/opengit/init.c
/home/brian/opengit/init.c: In function 'init_main':
/home/brian/opengit/init.c:162:10: warning: variable 'flags' set but not used [-Wunused-but-set-variable]
  uint8_t flags = 0;
          ^~~~~
/home/brian/opengit/init.c: In function 'init_dirinit':
/home/brian/opengit/init.c:106:4: warning: 'strncat' specified bound 1024 equals destination size [-Wstringop-overflow=]
    strncat(path, "/", PATH_MAX);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/brian/opengit/init.c: In function 'init_main':
/home/brian/opengit/init.c:196:4: warning: 'strncpy' output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
    strncpy(path, repodir, strlen(repodir));
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -MD -MP  -c /home/brian/opengit/lib/zlib-handler.c
egcc -O2 -pipe  -Wall -I/usr/local/include  -MD -MP  -c /home/brian/opengit/lib/buffering.c
egcc -O2 -pipe  -Wall -I/usr/local/include  -MD -MP  -c /home/brian/opengit/lib/loose.c
In file included from /home/brian/opengit/lib/loose.c:40:
/home/brian/opengit/lib/pack.h:55:20: warning: 'object_name' defined but not used [-Wunused-variable]
 static const char *object_name[] = {
                    ^~~~~~~~~~~
/home/brian/opengit/lib/loose.c: In function 'loose_content_handler':
/home/brian/opengit/lib/loose.c:53:46: warning: '/objects/' directive output may be truncated writing 9 bytes into a region of size between 1 and 1024 [-Wformat-truncation=]
  snprintf(objectpath, sizeof(objectpath), "%s/objects/%c%c/%s", dotgitpath, sha[0], sha[1], sha+2);
                                              ^~~~~~~~~
/home/brian/opengit/lib/loose.c:53:2: note: 'snprintf' output 13 or more bytes (assuming 1036) into a destination of size 1024
  snprintf(objectpath, sizeof(objectpath), "%s/objects/%c%c/%s", dotgitpath, sha[0], sha[1], sha+2);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -MD -MP  -c /home/brian/opengit/hash-object.c
/home/brian/opengit/hash-object.c: In function 'hash_object_create_zlib':
/home/brian/opengit/hash-object.c:114:42: warning: '/objects/' directive output may be truncated writing 9 bytes into a region of size between 1 and 1024 [-Wformat-truncation=]
  snprintf(filepath, sizeof(filepath), "%s/objects/%c%c", dotgitpath,
                                          ^~~~~~~~~
/home/brian/opengit/hash-object.c:114:2: note: 'snprintf' output between 12 and 1035 bytes into a destination of size 1024
  snprintf(filepath, sizeof(filepath), "%s/objects/%c%c", dotgitpath,
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      checksum[0], checksum[1]);
      ~~~~~~~~~~~~~~~~~~~~~~~~~
/home/brian/opengit/hash-object.c: In function 'hash_object_create_file':
/home/brian/opengit/hash-object.c:172:46: warning: '/objects/' directive output may be truncated writing 9 bytes into a region of size between 1 and 1024 [-Wformat-truncation=]
  snprintf(objectpath, sizeof(objectpath), "%s/objects/%c%c",
                                              ^~~~~~~~~
/home/brian/opengit/hash-object.c:172:2: note: 'snprintf' output between 12 and 1035 bytes into a destination of size 1024
  snprintf(objectpath, sizeof(objectpath), "%s/objects/%c%c",
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      dotgitpath, checksum[0], checksum[1]);
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/brian/opengit/hash-object.c:178:46: warning: '/objects/' directive output may be truncated writing 9 bytes into a region of size between 1 and 1024 [-Wformat-truncation=]
  snprintf(objectpath, sizeof(objectpath), "%s/objects/%c%c/%s",
                                              ^~~~~~~~~
/home/brian/opengit/hash-object.c:178:2: note: 'snprintf' output 13 or more bytes (assuming 1036) into a destination of size 1024
  snprintf(objectpath, sizeof(objectpath), "%s/objects/%c%c/%s",
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      dotgitpath, checksum[0], checksum[1], checksum+2);
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -MD -MP  -c /home/brian/opengit/update-index.c
/home/brian/opengit/update-index.c: In function 'update_index_open_index':
/home/brian/opengit/update-index.c:62:44: warning: '/index' directive output may be truncated writing 6 bytes into a region of size between 1 and 1024 [-Wformat-truncation=]
  snprintf(indexpath, sizeof(indexpath), "%s/index", dotgitpath);
                                            ^~~~~~
/home/brian/opengit/update-index.c:62:2: note: 'snprintf' output between 7 and 1030 bytes into a destination of size 1024
  snprintf(indexpath, sizeof(indexpath), "%s/index", dotgitpath);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -MD -MP  -c /home/brian/opengit/cat-file.c
/home/brian/opengit/cat-file.c: In function 'cat_file_main':
/home/brian/opengit/cat-file.c:223:4: warning: 'sha_str' may be used uninitialized in this function [-Wmaybe-uninitialized]
    cat_file_get_content(sha_str, flags);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -MD -MP  -c /home/brian/opengit/log.c
/home/brian/opengit/log.c: In function 'log_print_commit_headers':
/home/brian/opengit/log.c:78:7: warning: unused variable 'author' [-Wunused-variable]
  char author[255];
       ^~~~~~
/home/brian/opengit/log.c: In function 'log_display_cb':
/home/brian/opengit/log.c:135:18: warning: variable 'oldsize' set but not used [-Wunused-but-set-variable]
  int offset = 0, oldsize;
                  ^~~~~~~
In file included from /home/brian/opengit/log.c:41:
At top level:
/home/brian/opengit/lib/pack.h:55:20: warning: 'object_name' defined but not used [-Wunused-variable]
 static const char *object_name[] = {
                    ^~~~~~~~~~~
/home/brian/opengit/log.c: In function 'log_get_start_sha':
/home/brian/opengit/log.c:175:42: warning: '/HEAD' directive output may be truncated writing 5 bytes into a region of size between 1 and 1024 [-Wformat-truncation=]
  snprintf(headfile, sizeof(headfile), "%s/HEAD", dotgitpath);
                                          ^~~~~
/home/brian/opengit/log.c:175:2: note: 'snprintf' output between 6 and 1029 bytes into a destination of size 1024
  snprintf(headfile, sizeof(headfile), "%s/HEAD", dotgitpath);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/brian/opengit/log.c:188:42: warning: '%s' directive output may be truncated writing up to 39 bytes into a region of size between 0 and 1023 [-Wformat-truncation=]
   snprintf(refpath, sizeof(refpath), "%s/%s", dotgitpath, ref);
                                          ^~               ~~~
/home/brian/opengit/log.c:188:3: note: 'snprintf' output between 2 and 1064 bytes into a destination of size 1024
   snprintf(refpath, sizeof(refpath), "%s/%s", dotgitpath, ref);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/brian/opengit/log.c: In function 'log_get_loose_object':
/home/brian/opengit/log.c:214:46: warning: '/objects/' directive output may be truncated writing 9 bytes into a region of size between 1 and 1024 [-Wformat-truncation=]
  snprintf(objectpath, sizeof(objectpath), "%s/objects/%c%c/%s",
                                              ^~~~~~~~~
/home/brian/opengit/log.c:214:2: note: 'snprintf' output 13 or more bytes (assuming 1036) into a destination of size 1024
  snprintf(objectpath, sizeof(objectpath), "%s/objects/%c%c/%s",
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      dotgitpath, logarg->sha[0], logarg->sha[1],
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      logarg->sha+2);
      ~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -MD -MP  -c /home/brian/opengit/clone.c
/home/brian/opengit/clone.c: In function 'clone_pack_protocol_process':
/home/brian/opengit/clone.c:267:6: warning: variable 'check' set but not used [-Wunused-but-set-variable]
  int check;
      ^~~~~
/home/brian/opengit/clone.c: In function 'clone_http_get_sha':
/home/brian/opengit/clone.c:352:6: warning: variable 'content_length' set but not used [-Wunused-but-set-variable]
  int content_length;
      ^~~~~~~~~~~~~~
/home/brian/opengit/clone.c: In function 'generate_tree_item':
/home/brian/opengit/clone.c:606:19: warning: variable 'loosearg' set but not used [-Wunused-but-set-variable]
   struct loosearg loosearg;
                   ^~~~~~~~
/home/brian/opengit/clone.c: In function 'clone_http':
/home/brian/opengit/clone.c:452:2: warning: 'strncpy' output truncated before terminating nul copying 24 bytes from a string of the same length [-Wstringop-truncation]
  strncpy(suffix, "/.git/objects/pack/pack-", 24);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/brian/opengit/clone.c: In function 'clone_initial_config':
/home/brian/opengit/clone.c:548:2: warning: 'strncat' specified bound 12 equals source length [-Wstringop-overflow=]
  strncat(path, "/.git/config", 12);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/brian/opengit/clone.c:46:
At top level:
/home/brian/opengit/lib/pack.h:55:20: warning: 'object_name' defined but not used [-Wunused-variable]
 static const char *object_name[] = {
                    ^~~~~~~~~~~
/home/brian/opengit/clone.c: In function 'clone_http':
/home/brian/opengit/clone.c:409:2: warning: 'strncpy' output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
  strncpy(path, repodir, pathlen);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/brian/opengit/clone.c:408:12: note: length computed here
  pathlen = strlen(repodir);
            ^~~~~~~~~~~~~~~
/home/brian/opengit/clone.c:410:2: warning: 'strncpy' output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
  strncpy(srcpath, repodir, pathlen);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/brian/opengit/clone.c:408:12: note: length computed here
  pathlen = strlen(repodir);
            ^~~~~~~~~~~~~~~
/home/brian/opengit/clone.c: In function 'clone_initial_config':
/home/brian/opengit/clone.c:547:2: warning: 'strncpy' specified bound depends on the length of the source argument [-Wstringop-overflow=]
  strncpy(path, repodir, strlen(repodir)+1);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/brian/opengit/clone.c:547:25: note: length computed here
  strncpy(path, repodir, strlen(repodir)+1);
                         ^~~~~~~~~~~~~~~
egcc -O2 -pipe  -Wall -I/usr/local/include  -MD -MP  -c /home/brian/opengit/index-pack.c
In file included from /home/brian/opengit/index-pack.c:46:
/home/brian/opengit/lib/pack.h:55:20: warning: 'object_name' defined but not used [-Wunused-variable]
 static const char *object_name[] = {
                    ^~~~~~~~~~~
egcc -lz -L/usr/local/lib -lfetch  -o ogit ogit.o ini.o index.o common.o pack.o remote.o init.o zlib-handler.o buffering.o loose.o hash-object.o update-index.o cat-file.o log.o clone.o index-pack.o

With that said, I can successfully clone one of my own repos with:

$ ogit clone https://github.com/ibara/shuf.git
khanzf commented 5 years ago

Thank you very much for the comments. Can you please verify that this (f7eb0d21ebaa25af33c9238afab05c469ecd7922) is the right direction I should be headed? The problem I frequently ran into is, dotgitpath's size is PATH_MAX. When I construct path that appends to dotgitpath, its size will also be PATH_MAX, thus resulting in an overflow condition.

For example:

char path[PATH_MAX]; snprintf(path, PATH_MAX, "%s/foobar", dotgitpath);

If dotgitpath is at or above PATH_MAX - strlen("/foobar"), this will trigger an overflow condition.

Yes, the clone works, but I still need to produce the .git/index file, but there is a good amount of cleanup left to do before I get there. It will also help me going forward to correct my style.

kevans91 commented 5 years ago

Thank you very much for the comments. Can you please verify that this (f7eb0d2) is the right direction I should be headed? The problem I frequently ran into is, dotgitpath's size is PATH_MAX. When I construct path that appends to dotgitpath, its size will also be PATH_MAX, thus resulting in an overflow condition.

For example:

char path[PATH_MAX]; snprintf(path, PATH_MAX, "%s/foobar", dotgitpath);

If dotgitpath is at or above PATH_MAX - strlen("/foobar"), this will trigger an overflow condition.

To be pedantic, this will not trigger an overflow -- snprintf is documented to write size - 1characters and null terminate on the sizeth character. path here will end up truncated and the return value of snprintf will be >= PATH_MAX for you act appropriately on. snprintf should be fine for these cases as long as the source is properly null terminated.

Those strncpy/strncat should be converted to strlcpy/strlcat for a behavior that's a lot more sane if the destination is ever a string that could potentially be printed out somewhere or otherwise used in any way that's expecting proper null termination, because strn* won't guarantee it.

Yes, the clone works, but I still need to produce the .git/index file, but there is a good amount of cleanup left to do before I get there. It will also help me going forward to correct my style.

ibara commented 5 years ago

Thank you very much for the comments. Can you please verify that this (f7eb0d2) is the right direction I should be headed? The problem I frequently ran into is, dotgitpath's size is PATH_MAX. When I construct path that appends to dotgitpath, its size will also be PATH_MAX, thus resulting in an overflow condition. For example:

char path[PATH_MAX]; snprintf(path, PATH_MAX, "%s/foobar", dotgitpath);

If dotgitpath is at or above PATH_MAX - strlen("/foobar"), this will trigger an overflow condition.

To be pedantic, this will not trigger an overflow -- snprintf is documented to write size - 1characters and null terminate on the sizeth character. path here will end up truncated and the return value of snprintf will be >= PATH_MAX for you act appropriately on. snprintf should be fine for these cases as long as the source is properly null terminated.

Right. That's why in my original diff I suggested a single buffer size of 4096, and I made all the static buffers that size. In one hand, you could say it's wasteful. On the other hand, we're talking about kilobytes and I don't imagine that this would ever be a problem in a practical setting. 4096 is guaranteed to be larger than could be used, and it shuts up a potentially overzealous gcc.

Those strncpy/strncat should be converted to strlcpy/strlcat for a behavior that's a lot more sane if the destination is ever a string that could potentially be printed out somewhere or otherwise used in any way that's expecting proper null termination, because strn* won't guarantee it.

Yes, the clone works, but I still need to produce the .git/index file, but there is a good amount of cleanup left to do before I get there. It will also help me going forward to correct my style.

khanzf commented 5 years ago

Just to be clear, would this entail something like: #define LARGEST_FILE_PATH 4096 and then replacing PATH_MAX with LARGEST_FILE_PATH? I don't see how that changes the overflow condition or why gcc would not complain (does it stop after a number of bytes?), other than making it less likely to accidentally trigger.

ibara commented 5 years ago

Not quite. It's not a blind replacement. See the ogit-fix-buffers.txt file earlier in the thread for a diff. Note where I choose to use OGIT_BUFSIZE. And, more importantly, where I do not.

ibara commented 5 years ago

By the way, you need to check and cap the repodir variable in init.c.