{"thread":{"id":"46551","subject":"fatal: Out of memory, getdelim failed under NFS mounts","startedAt":"2017-08-09T18:10:32Z","lastAt":"2017-08-13T05:59:01Z","messageCount":21,"participants":["Yaroslav Halchenko","René Scharfe","Junio C Hamano","Jeff King","Simon Ruderich"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"325976","messageId":"20170809173928.h2ylvg5tp2p5inem@hopa.kiewit.dartmouth.edu","threadId":"46551","inReplyTo":null,"subject":"fatal: Out of memory, getdelim failed under NFS mounts","fromName":"Yaroslav Halchenko","fromEmail":"yoh@onerussian.com","sentAt":"2017-08-09T17:39:28Z","receivedAt":"2017-08-09T18:10:32Z","isPatch":false,"sender":{"key":"yoh@onerussian.com","avatar":"https://gravatar.com/avatar/8901b82415ae451a83aea49409708912726e53620e3ac92320bf1f86548d97e9?d=mp&s=160"},"body":"Dear Git gurus,\n\nMore context (may be different issue(s)) could be found at \nhttp://git-annex.branchable.com/forum/git-annex_add_out_of_memory_error/\nbut currently I am consistently reproducing it while running \ngit (1:2.11.0-3 debian stretch build) within debian stretch singularity\nenvironment [1].  \n\nExternal system is Centos 6.9, and git 1.7.1 (and installed in modules\n2.0.4) do not show similar buggy behavior.\n\nNFS mounted partitions are bind mounted inside the sinularity space and\nwhen I try to do some git operations, I get that error inconsistently , e.g.\n\n\tyhalchen@discovery:/mnt/scratch/yoh/datalad$ git pull --ff-only origin master\n\tfatal: Out of memory, getdelim failed\n\terror: git://github.com/datalad/datalad did not send all necessary objects\n\n\tyhalchen@discovery:/mnt/scratch/yoh/datalad$ git pull --ff-only origin master\n\tfatal: Out of memory, getdelim failed\n\terror: git://github.com/datalad/datalad did not send all necessary objects\n\n\tyhalchen@discovery:/mnt/scratch/yoh/datalad$ git pull --ff-only origin master\n\tFrom git://github.com/datalad/datalad\n\t * branch              master     -> FETCH_HEAD\n\tfatal: Out of memory, getdelim failed\n\nand some times it succeeds.  So it smells that some race condition\nsomewhere...?\n\nany recommendations on how to pin point the \"offender\"? ;)  Here is the\ntrailer of one of the straced calls:\n\n...\n[pid 19713] getcwd(\"/ihome/yhalchen/datalad\", 129) = 24\n[pid 19713] stat(\".git\", {st_mode=S_IFDIR|0755, st_size=322, ...}) = 0\n[pid 19713] lstat(\".git/HEAD\", {st_mode=S_IFREG|0644, st_size=41, ...}) = 0\n[pid 19713] open(\".git/HEAD\", O_RDONLY) = 3\n[pid 19713] read(3, \"39f80454d31cfb691b006302b1f29dee\"..., 255) = 41\n[pid 19713] read(3, \"\", 214)            = 0\n[pid 19713] close(3)                    = 0\n[pid 19713] lstat(\".git/commondir\", 0x7ffc1a571190) = -1 ENOENT (No such file or directory)\n[pid 19713] access(\".git/objects\", X_OK) = 0\n[pid 19713] access(\".git/refs\", X_OK)   = 0\n[pid 19713] lstat(\".git/commondir\", 0x7ffc1a571120) = -1 ENOENT (No such file or directory)\n[pid 19713] open(\".git/config\", O_RDONLY) = 3\n[pid 19713] fstat(3, {st_mode=S_IFREG|0644, st_size=257, ...}) = 0\n[pid 19713] mmap(NULL, 528384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x2ba7894e7000\n[pid 19713] read(3, \"[core]\\n\\trepositoryformatversion \"..., 524288) = 257\n[pid 19713] read(3, \"\", 524288)         = 0\n[pid 19713] close(3)                    = 0\n[pid 19713] munmap(0x2ba7894e7000, 528384) = 0\n[pid 19713] stat(\".\", {st_mode=S_IFDIR|0755, st_size=907, ...}) = 0\n[pid 19713] getcwd(\"/ihome/yhalchen/datalad\", 129) = 24\n[pid 19713] chdir(\".\")                  = 0\n[pid 19713] getcwd(\"/ihome/yhalchen/datalad\", 130) = 24\n[pid 19713] lstat(\"/ihome/yhalchen/datalad\", {st_mode=S_IFDIR|0755, st_size=907, ...}) = 0\n[pid 19713] chdir(\"/ihome/yhalchen/datalad\") = 0\n[pid 19713] stat(\".git\", {st_mode=S_IFDIR|0755, st_size=322, ...}) = 0\n[pid 19713] lstat(\".git/commondir\", 0x7ffc1a571140) = -1 ENOENT (No such file or directory)\n[pid 19713] access(\"/etc/gitconfig\", R_OK) = -1 ENOENT (No such file or directory)\n[pid 19713] access(\"/ihome/yhalchen/.config/git/config\", R_OK) = -1 ENOENT (No such file or directory)\n[pid 19713] access(\"/ihome/yhalchen/.gitconfig\", R_OK) = -1 ENOENT (No such file or directory)\n[pid 19713] access(\".git/config\", R_OK) = 0\n[pid 19713] open(\".git/config\", O_RDONLY) = 3\n[pid 19713] fstat(3, {st_mode=S_IFREG|0644, st_size=257, ...}) = 0\n[pid 19713] brk(0x2ba78afea000)         = 0x2ba78afea000\n[pid 19713] read(3, \"[core]\\n\\trepositoryformatversion \"..., 524288) = 257\n[pid 19713] read(3, \"\", 524288)         = 0\n[pid 19713] close(3)                    = 0\n[pid 19713] access(\"/etc/gitconfig\", R_OK) = -1 ENOENT (No such file or directory)\n[pid 19713] access(\"/ihome/yhalchen/.config/git/config\", R_OK) = -1 ENOENT (No such file or directory)\n[pid 19713] access(\"/ihome/yhalchen/.gitconfig\", R_OK) = -1 ENOENT (No such file or directory)\n[pid 19713] access(\".git/config\", R_OK) = 0\n[pid 19713] open(\".git/config\", O_RDONLY) = 3\n[pid 19713] fstat(3, {st_mode=S_IFREG|0644, st_size=257, ...}) = 0\n[pid 19713] read(3, \"[core]\\n\\trepositoryformatversion \"..., 524288) = 257\n[pid 19713] read(3, \"\", 524288)         = 0\n[pid 19713] close(3)                    = 0\n[pid 19713] open(\".git/objects/pack\", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 3\n[pid 19713] fstat(3, {st_mode=S_IFDIR|0755, st_size=270, ...}) = 0\n[pid 19713] getdents(3, /* 6 entries */, 524288) = 336\n[pid 19713] access(\".git/objects/pack/pack-fd9e70075570d8ec41f12605852f54f1cb9771a8.keep\", F_OK) = -1 ENOENT (No such file or directory)\n[pid 19713] stat(\".git/objects/pack/pack-fd9e70075570d8ec41f12605852f54f1cb9771a8.pack\", {st_mode=S_IFREG|0444, st_size=10019975, ...}) = 0\n[pid 19713] access(\".git/objects/pack/pack-f1fc124e3aa1619d65a6ba56219f84871a762775.keep\", F_OK) = -1 ENOENT (No such file or directory)\n[pid 19713] stat(\".git/objects/pack/pack-f1fc124e3aa1619d65a6ba56219f84871a762775.pack\", {st_mode=S_IFREG|0444, st_size=610330, ...}) = 0\n[pid 19713] getdents(3, /* 0 entries */, 524288) = 0\n[pid 19713] close(3)                    = 0\n[pid 19713] getcwd(\"/ihome/yhalchen/datalad\", 129) = 24\n[pid 19713] open(\".git/objects/info/alternates\", O_RDONLY|O_NOATIME|O_CLOEXEC) = -1 ENOENT (No such file or directory)\n[pid 19713] open(\".git/objects/17\", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = -1 ENOENT (No such file or directory)\n[pid 19713] fstat(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 17), ...}) = 0\n[pid 19713] close(1)                    = 0\n[pid 19713] exit_group(0)               = ?\n[pid 19713] +++ exited with 0 +++\n[pid 19710] <... wait4 resumed> [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], 0, NULL) = 19713\n[pid 19710] --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=19713, si_uid=14076, si_status=0, si_utime=0, si_stime=0} ---\n[pid 19710] exit_group(1)               = ?\n[pid 19710] +++ exited with 1 +++\n<... wait4 resumed> [{WIFEXITED(s) && WEXITSTATUS(s) == 1}], 0, NULL) = 19710\n--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=19710, si_uid=14076, si_status=1, si_utime=0, si_stime=2} ---\nexit_group(1)                           = ?\n+++ exited with 1 +++\n\n\nPS please CC me in replies! Thanks in advance\n\n[1] http://datasets.datalad.org/singularity/neurodebian-v2.1.img.tgz\nit is a tarball to extract/run with , I promise I did no evil in there ;)\n\n-- \nYaroslav O. Halchenko\nCenter for Open Neuroscience     http://centerforopenneuroscience.org\nDartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755\nPhone: +1 (603) 646-9834                       Fax: +1 (603) 646-1419\nWWW:   http://www.linkedin.com/in/yarik        \n"},{"id":"326027","messageId":"8e307474-d180-6d98-2c6b-062f2181bd14@web.de","threadId":"46551","inReplyTo":"20170809173928.h2ylvg5tp2p5inem@hopa.kiewit.dartmouth.edu","subject":"Re: fatal: Out of memory, getdelim failed under NFS mounts","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-08-10T13:27:29Z","receivedAt":"2017-08-10T13:27:47Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.08.2017 um 19:39 schrieb Yaroslav Halchenko:\n> More context (may be different issue(s)) could be found at\n> http://git-annex.branchable.com/forum/git-annex_add_out_of_memory_error/\n> but currently I am consistently reproducing it while running\n> git (1:2.11.0-3 debian stretch build) within debian stretch singularity\n> environment [1].\n> \n> External system is Centos 6.9, and git 1.7.1 (and installed in modules\n> 2.0.4) do not show similar buggy behavior.\n> \n> NFS mounted partitions are bind mounted inside the sinularity space and\n> when I try to do some git operations, I get that error inconsistently , e.g.\n> \n> \tyhalchen@discovery:/mnt/scratch/yoh/datalad$ git pull --ff-only origin master\n> \tfatal: Out of memory, getdelim failed\n> \terror: git://github.com/datalad/datalad did not send all necessary objects\n> \n> \tyhalchen@discovery:/mnt/scratch/yoh/datalad$ git pull --ff-only origin master\n> \tfatal: Out of memory, getdelim failed\n> \terror: git://github.com/datalad/datalad did not send all necessary objects\n> \n> \tyhalchen@discovery:/mnt/scratch/yoh/datalad$ git pull --ff-only origin master\n> \tFrom git://github.com/datalad/datalad\n> \t * branch              master     -> FETCH_HEAD\n> \tfatal: Out of memory, getdelim failed\n> \n> and some times it succeeds.  So it smells that some race condition\n> somewhere...?\n\nI doubt the type of file system matters.  The questions are: How much\nmain memory do you have, what is git trying to cram into it, is there\na way to reduce the memory footprint or do you need to add more RAM?\n\n> any recommendations on how to pin point the \"offender\"? ;)\nRunning \"GIT_TRACE=1 git pull --ff-only origin master\" would be a\ngood start, I think, to find out which of the different activities\nthat pull is doing causes the out-of-memory error.\n\n\"free\" and \"ulimit -a\" can help you find out how much memory you can\nuse.\n\nAlso: What does \"wc -L .git/FETCH_HEAD .git/packed-refs\" report?\ngetdelim() is used mostly to read lines from files like these and in\nthe admittedly unlikely case that they are *really* long such an\nerror would be expected.\n\nRené\n"},{"id":"326028","messageId":"20170810144305.5d4lcx6oubkemsss@hopa.kiewit.dartmouth.edu","threadId":"46551","inReplyTo":"8e307474-d180-6d98-2c6b-062f2181bd14@web.de","subject":"Re: fatal: Out of memory, getdelim failed under NFS mounts","fromName":"Yaroslav Halchenko","fromEmail":"yoh@onerussian.com","sentAt":"2017-08-10T14:43:06Z","receivedAt":"2017-08-10T14:43:21Z","isPatch":false,"sender":{"key":"yoh@onerussian.com","avatar":"https://gravatar.com/avatar/8901b82415ae451a83aea49409708912726e53620e3ac92320bf1f86548d97e9?d=mp&s=160"},"body":"Thank you René!  comments/answers embedded below\n\nOn Thu, 10 Aug 2017, René Scharfe wrote:\n\n> Am 09.08.2017 um 19:39 schrieb Yaroslav Halchenko:\n> > More context (may be different issue(s)) could be found at\n> > http://git-annex.branchable.com/forum/git-annex_add_out_of_memory_error/\n> > but currently I am consistently reproducing it while running\n> > git (1:2.11.0-3 debian stretch build) within debian stretch singularity\n> > environment [1].\n\n> > External system is Centos 6.9, and git 1.7.1 (and installed in modules\n> > 2.0.4) do not show similar buggy behavior.\n\n> > NFS mounted partitions are bind mounted inside the sinularity space and\n> > when I try to do some git operations, I get that error inconsistently , e.g.\n\n> > \tyhalchen@discovery:/mnt/scratch/yoh/datalad$ git pull --ff-only origin master\n> > \tfatal: Out of memory, getdelim failed\n> > \terror: git://github.com/datalad/datalad did not send all necessary objects\n\n> > \tyhalchen@discovery:/mnt/scratch/yoh/datalad$ git pull --ff-only origin master\n> > \tfatal: Out of memory, getdelim failed\n> > \terror: git://github.com/datalad/datalad did not send all necessary objects\n\n> > \tyhalchen@discovery:/mnt/scratch/yoh/datalad$ git pull --ff-only origin master\n> > \tFrom git://github.com/datalad/datalad\n> > \t * branch              master     -> FETCH_HEAD\n> > \tfatal: Out of memory, getdelim failed\n\n> > and some times it succeeds.  So it smells that some race condition\n> > somewhere...?\n\n> I doubt the type of file system matters.  \n\nSo far it has been a very consistent indicator.  I did not manage to get\nthis error while performing the same operation under /tmp (bind to local\nmounted drive), where it also feels going faster (again suggesting that\noriginal issue is some kind of a race)\n\n> The questions are: How much\n> main memory do you have, what is git trying to cram into it, is there\n> a way to reduce the memory footprint or do you need to add more RAM?\n> ... reordered ...\n> \"free\" and \"ulimit -a\" can help you find out how much memory you can\n> use.\n\nI think those aren't the reason:\n\nyhalchen@discovery:/mnt/scratch/yoh/datalad$ free -h\n              total        used        free      shared  buff/cache   available\nMem:           126G        2.5G         90G        652K         33G        123G\nSwap:          127G        1.7M        127G\nyhalchen@discovery:/mnt/scratch/yoh/datalad$ ulimit\nunlimited\n\n> > any recommendations on how to pin point the \"offender\"? ;)\n> Running \"GIT_TRACE=1 git pull --ff-only origin master\" would be a\n> good start, I think, to find out which of the different activities\n> that pull is doing causes the out-of-memory error.\n\nsamples of bad, and then good runs (from eyeballing -- the same until\nerror message):\n\nyhalchen@discovery:/mnt/scratch/yoh$ cat git_trace_bad.log\n14:05:25.782270 git.c:371               trace: built-in: git 'pull' '--ff-only' 'origin' 'master'\n14:05:25.795036 run-command.c:350       trace: run_command: 'fetch' '--update-head-ok' 'origin' 'master'\n14:05:25.795332 exec_cmd.c:116          trace: exec: 'git' 'fetch' '--update-head-ok' 'origin' 'master'\n14:05:25.797212 git.c:371               trace: built-in: git 'fetch' '--update-head-ok' 'origin' 'master'\n14:05:25.904088 run-command.c:350       trace: run_command: 'rev-list' '--objects' '--stdin' '--not' '--all' '--quiet'\n14:05:26.085954 run-command.c:350       trace: run_command: 'index-pack' '--stdin' '--fix-thin' '--keep=fetch-pack 11652 on discovery.hpcc.dartmouth.edu' '--pack_header=2,103'\n14:05:26.086333 exec_cmd.c:116          trace: exec: 'git' 'index-pack' '--stdin' '--fix-thin' '--keep=fetch-pack 11652 on discovery.hpcc.dartmouth.edu' '--pack_header=2,103'\n14:05:26.088382 git.c:371               trace: built-in: git 'index-pack' '--stdin' '--fix-thin' '--keep=fetch-pack 11652 on discovery.hpcc.dartmouth.edu' '--pack_header=2,103'\n14:05:26.133326 run-command.c:350       trace: run_command: 'rev-list' '--objects' '--stdin' '--not' '--all' '--quiet'\n14:05:26.133688 exec_cmd.c:116          trace: exec: 'git' 'rev-list' '--objects' '--stdin' '--not' '--all' '--quiet'\n14:05:26.135493 git.c:371               trace: built-in: git 'rev-list' '--objects' '--stdin' '--not' '--all' '--quiet'\nfatal: Out of memory, getdelim failed\nerror: git://github.com/datalad/datalad did not send all necessary objects\n\n14:05:26.138838 run-command.c:350       trace: run_command: 'gc' '--auto'\n14:05:26.139131 exec_cmd.c:116          trace: exec: 'git' 'gc' '--auto'\n14:05:26.141108 git.c:371               trace: built-in: git 'gc' '--auto'\n\n\nyhalchen@discovery:/mnt/scratch/yoh$ cat git_trace_good.log\n14:05:37.851862 git.c:371               trace: built-in: git 'pull' '--ff-only' 'origin' 'master'\n14:05:37.854250 run-command.c:350       trace: run_command: 'fetch' '--update-head-ok' 'origin' 'master'\n14:05:37.854527 exec_cmd.c:116          trace: exec: 'git' 'fetch' '--update-head-ok' 'origin' 'master'\n14:05:37.856389 git.c:371               trace: built-in: git 'fetch' '--update-head-ok' 'origin' 'master'\n14:05:37.954099 run-command.c:350       trace: run_command: 'rev-list' '--objects' '--stdin' '--not' '--all' '--quiet'\n14:05:38.118652 run-command.c:350       trace: run_command: 'index-pack' '--stdin' '--fix-thin' '--keep=fetch-pack 11688 on discovery.hpcc.dartmouth.edu' '--pack_header=2,103'\n14:05:38.119011 exec_cmd.c:116          trace: exec: 'git' 'index-pack' '--stdin' '--fix-thin' '--keep=fetch-pack 11688 on discovery.hpcc.dartmouth.edu' '--pack_header=2,103'\n14:05:38.120924 git.c:371               trace: built-in: git 'index-pack' '--stdin' '--fix-thin' '--keep=fetch-pack 11688 on discovery.hpcc.dartmouth.edu' '--pack_header=2,103'\n14:05:38.167508 run-command.c:350       trace: run_command: 'rev-list' '--objects' '--stdin' '--not' '--all' '--quiet'\n14:05:38.167851 exec_cmd.c:116          trace: exec: 'git' 'rev-list' '--objects' '--stdin' '--not' '--all' '--quiet'\n14:05:38.169726 git.c:371               trace: built-in: git 'rev-list' '--objects' '--stdin' '--not' '--all' '--quiet'\nFrom git://github.com/datalad/datalad\n * branch              master     -> FETCH_HEAD\n   39f80454..1f90ef47  master     -> origin/master\n14:05:38.306113 run-command.c:1130      run_processes_parallel: preparing to run up to 1 tasks\n14:05:38.306148 run-command.c:1162      run_processes_parallel: done\n14:05:38.306313 run-command.c:350       trace: run_command: 'gc' '--auto'\n14:05:38.306616 exec_cmd.c:116          trace: exec: 'git' 'gc' '--auto'\n14:05:38.308598 git.c:371               trace: built-in: git 'gc' '--auto'\n14:05:38.311380 run-command.c:350       trace: run_command: 'merge' '--ff-only' 'FETCH_HEAD'\n14:05:38.311645 exec_cmd.c:116          trace: exec: 'git' 'merge' '--ff-only' 'FETCH_HEAD'\n14:05:38.313384 git.c:371               trace: built-in: git 'merge' '--ff-only' 'FETCH_HEAD'\n14:05:45.092247 run-command.c:350       trace: run_command: 'gc' '--auto'\nUpdating 39f80454..1f90ef47\nFast-forward\n14:05:45.092620 exec_cmd.c:116          trace: exec: 'git' 'gc' '--auto'\n14:05:45.095862 git.c:371               trace: built-in: git 'gc' '--auto'\n\n\n> Also: What does \"wc -L .git/FETCH_HEAD .git/packed-refs\" report?\n\n\"varying\" and not consistent with causing an error (first trials, where I\ndid not cat .git/FETCH_HEAD kinda suggested differently):\n\nyhalchen@discovery:/mnt/scratch/yoh/datalad$ cat .git/FETCH_HEAD; wc -L .git/FETCH_HEAD .git/packed-refs; git pull --ff-only origin master\n1f90ef474ee200befea19ba77242fa44f16739f0                branch 'master' of git://github.com/datalad/datalad\n 107 .git/FETCH_HEAD\n  90 .git/packed-refs\n 107 total\nFrom git://github.com/datalad/datalad\n * branch              master     -> FETCH_HEAD\nAlready up-to-date.\nyhalchen@discovery:/mnt/scratch/yoh/datalad$ cat .git/FETCH_HEAD; wc -L .git/FETCH_HEAD .git/packed-refs; git pull --ff-only origin master \n1f90ef474ee200befea19ba77242fa44f16739f0                branch 'master' of git://github.com/datalad/datalad\n 107 .git/FETCH_HEAD\n  90 .git/packed-refs\n 107 total\nfatal: Out of memory, getdelim failed\nerror: git://github.com/datalad/datalad did not send all necessary objects\n\nyhalchen@discovery:/mnt/scratch/yoh/datalad$ cat .git/FETCH_HEAD; wc -L .git/FETCH_HEAD .git/packed-refs; git pull --ff-only origin master \n   0 .git/FETCH_HEAD\n  90 .git/packed-refs\n  90 total\nfatal: Out of memory, getdelim failed\nerror: git://github.com/datalad/datalad did not send all necessary objects\n\nyhalchen@discovery:/mnt/scratch/yoh/datalad$ cat .git/FETCH_HEAD; wc -L .git/FETCH_HEAD .git/packed-refs; git pull --ff-only origin master \n   0 .git/FETCH_HEAD\n  90 .git/packed-refs\n  90 total\nfatal: Out of memory, getdelim failed\nerror: git://github.com/datalad/datalad did not send all necessary objects\n\nyhalchen@discovery:/mnt/scratch/yoh/datalad$ cat .git/FETCH_HEAD; wc -L .git/FETCH_HEAD .git/packed-refs; git pull --ff-only origin master \n   0 .git/FETCH_HEAD\n  90 .git/packed-refs\n  90 total\nFrom git://github.com/datalad/datalad\n * branch              master     -> FETCH_HEAD\nAlready up-to-date.\n\n\n> getdelim() is used mostly to read lines from files like these and in\n> the admittedly unlikely case that they are *really* long such an\n> error would be expected.\n\npacked-ref pointer seems to also relate to the strace output around the\npoint of message:\n\n[pid 12843] open(\".git/packed-refs\", O_RDONLY) = 3\n[pid 12843] fstat(3, {st_mode=S_IFREG|0644, st_size=5042, ...}) = 0\n[pid 12843] fstat(3, {st_mode=S_IFREG|0644, st_size=5042, ...}) = 0\n[pid 12843] brk(0x2b5704317000)         = 0x2b5704276000\n[pid 12843] mmap(NULL, 1048576, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x2b570451d000\n[pid 12843] read(3, \"# pack-refs with: peeled fully-p\"..., 524288) = 5042\n[pid 12843] read(3, \"\", 524288)         = 0\n[pid 12843] fstat(2, {st_mode=S_IFIFO|0600, st_size=0, ...}) = 0\n[pid 12843] write(2, \"fatal: Out of memory, getdelim f\"..., 38fatal: Out of memory, getdelim failed\n) = 38\n[pid 12843] lseek(0, -41, SEEK_CUR)     = -1 ESPIPE (Illegal seek)\n[pid 12843] exit_group(128)             = ?\n\n\nI verified that content of that packed-refs didn't change between good and bad\nruns.\n\nNB is there a diff which could be given regexes within a line to ignore\nin diffs in so we could still retain original lines, i.e. answer to\nhttps://stackoverflow.com/questions/15841223/diff-while-ignoring-patterns-within-a-line-but-not-the-entire-line\n?\n\nFWIW -- here is a diff (from good to bad run) of strace (with pid/address\ninfo changed to stay the same for comparisons):\n\n@@ -3121,6 +3118,7 @@\n [pid YYYYY] close(3)                    = 0\n [pid YYYYY] brk(NULL)                   = 0xXXXXX\n [pid YYYYY] brk(0xXXXXX)         = 0xXXXXX\n+[pid YYYYY] mmap(NULL, 1048576, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xXXXXX\n [pid YYYYY] open(\"/usr/lib/locale/locale-archive\", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)\n [pid YYYYY] open(\"/usr/share/locale/locale.alias\", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)\n [pid YYYYY] open(\"/usr/lib/locale/en_US/LC_MESSAGES\", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)\n@@ -3144,11 +3142,9 @@\n [pid YYYYY] lstat(\".git/commondir\", 0xXXXXX) = -1 ENOENT (No such file or directory)\n [pid YYYYY] open(\".git/config\", O_RDONLY) = 3\n [pid YYYYY] fstat(3, {st_mode=S_IFREG|0644, st_size=257, ...}) = 0\n-[pid YYYYY] mmap(NULL, 528384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xXXXXX\n [pid YYYYY] read(3, \"[core]\\n\\trepositoryformatversion \"..., 524288) = 257\n [pid YYYYY] read(3, \"\", 524288)         = 0\n [pid YYYYY] close(3)                    = 0\n-[pid YYYYY] munmap(0xXXXXX, 528384) = 0\n [pid YYYYY] stat(\".\", {st_mode=S_IFDIR|0755, st_size=907, ...}) = 0\n [pid YYYYY] getcwd(\"/mnt/scratch/yoh/datalad\", 129) = 25\n [pid YYYYY] chdir(\".\")                  = 0\n@@ -3163,7 +3159,6 @@\n [pid YYYYY] access(\".git/config\", R_OK) = 0\n [pid YYYYY] open(\".git/config\", O_RDONLY) = 3\n [pid YYYYY] fstat(3, {st_mode=S_IFREG|0644, st_size=257, ...}) = 0\n-[pid YYYYY] brk(0xXXXXX)         = 0xXXXXX\n [pid YYYYY] read(3, \"[core]\\n\\trepositoryformatversion \"..., 524288) = 257\n [pid YYYYY] read(3, \"\", 524288)         = 0\n [pid YYYYY] close(3)                    = 0\n@@ -3180,245 +3175,48 @@\n [pid YYYYY] read(0, \"1f90ef474ee200befea19ba77242fa44\"..., 4096) = 82\n [pid YYYYY] open(\".git/refs/\", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 3\n [pid YYYYY] fstat(3, {st_mode=S_IFDIR|0755, st_size=70, ...}) = 0\n-[pid YYYYY] brk(0xXXXXX)         = 0xXXXXX\n+[pid YYYYY] mmap(NULL, 528384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xXXXXX\n [pid YYYYY] getdents(3, /* 5 entries */, 524288) = 136\n [pid YYYYY] stat(\".git/refs/heads\", {st_mode=S_IFDIR|0755, st_size=24, ...}) = 0\n [pid YYYYY] stat(\".git/refs/remotes\", {st_mode=S_IFDIR|0755, st_size=24, ...}) = 0\n [pid YYYYY] stat(\".git/refs/tags\", {st_mode=S_IFDIR|0755, st_size=47, ...}) = 0\n [pid YYYYY] getdents(3, /* 0 entries */, 524288) = 0\n+[pid YYYYY] munmap(0xXXXXX, 528384) = 0\n [pid YYYYY] close(3)                    = 0\n [pid YYYYY] open(\".git/refs/bisect\", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = -1 ENOENT (No such file or directory)\n [pid YYYYY] open(\".git/packed-refs\", O_RDONLY) = 3\n [pid YYYYY] fstat(3, {st_mode=S_IFREG|0644, st_size=5042, ...}) = 0\n [pid YYYYY] fstat(3, {st_mode=S_IFREG|0644, st_size=5042, ...}) = 0\n-[pid YYYYY] read(3, \"# pack-refs with: peeled fully-p\"..., 524288) = 5042\n-[pid YYYYY] read(3, \"\", 524288)         = 0\n-[pid YYYYY] close(3)                    = 0\n-[pid YYYYY] open(\".git/objects/pack\", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 3\n-[pid YYYYY] fstat(3, {st_mode=S_IFDIR|0755, st_size=405, ...}) = 0\n-[pid YYYYY] getdents(3, /* 8 entries */, 524288) = 480\n-[pid YYYYY] access(\".git/objects/pack/pack-a5ed9c83e2a39b21a2ab12dc351d2a7513d76e48.keep\", F_OK) = -1 ENOENT (No such file or directory)\n-[pid YYYYY] stat(\".git/objects/pack/pack-a5ed9c83e2a39b21a2ab12dc351d2a7513d76e48.pack\", {st_mode=S_IFREG|0444, st_size=124835, ...}) = 0\n-[pid YYYYY] access(\".git/objects/pack/pack-fd9e70075570d8ec41f12605852f54f1cb9771a8.keep\", F_OK) = -1 ENOENT (No such file or directory)\n-[pid YYYYY] stat(\".git/objects/pack/pack-fd9e70075570d8ec41f12605852f54f1cb9771a8.pack\", {st_mode=S_IFREG|0444, st_size=10019975, ...}) = 0\n-[pid YYYYY] access(\".git/objects/pack/pack-f1fc124e3aa1619d65a6ba56219f84871a762775.keep\", F_OK) = -1 ENOENT (No such file or directory)\n-[pid YYYYY] stat(\".git/objects/pack/pack-f1fc124e3aa1619d65a6ba56219f84871a762775.pack\", {st_mode=S_IFREG|0444, st_size=610330, ...}) = 0\n\n\n\n\n-- \nYaroslav O. Halchenko\nCenter for Open Neuroscience     http://centerforopenneuroscience.org\nDartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755\nPhone: +1 (603) 646-9834                       Fax: +1 (603) 646-1419\nWWW:   http://www.linkedin.com/in/yarik        \n"},{"id":"326068","messageId":"xmqqzib72qvs.fsf@gitster.mtv.corp.google.com","threadId":"46551","inReplyTo":"8e307474-d180-6d98-2c6b-062f2181bd14@web.de","subject":"Re: fatal: Out of memory, getdelim failed under NFS mounts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-10T18:56:39Z","receivedAt":"2017-08-10T18:57:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> I doubt the type of file system matters.  The questions are: How much\n> main memory do you have, what is git trying to cram into it, is there\n> a way to reduce the memory footprint or do you need to add more RAM?\n>\n>> any recommendations on how to pin point the \"offender\"? ;)\n> Running \"GIT_TRACE=1 git pull --ff-only origin master\" would be a\n> good start, I think, to find out which of the different activities\n> that pull is doing causes the out-of-memory error.\n>\n> \"free\" and \"ulimit -a\" can help you find out how much memory you can\n> use.\n>\n> Also: What does \"wc -L .git/FETCH_HEAD .git/packed-refs\" report?\n> getdelim() is used mostly to read lines from files like these and in\n> the admittedly unlikely case that they are *really* long such an\n> error would be expected.\n\nThere is only one getdelim() call, which was introduced in v2.5.0\ntimeframe, and it is used like this:\n\n\tr = getdelim(&sb->buf, &sb->alloc, term, fp);\n\n\tif (r > 0) {\n\t\tsb->len = r;\n\t\treturn 0;\n\t}\n\tassert(r == -1);\n\n\t/*\n\t * Normally we would have called xrealloc, which will try to free\n\t * memory and recover. But we have no way to tell getdelim() to do so.\n\t * Worse, we cannot try to recover ENOMEM ourselves, because we have\n\t * no idea how many bytes were read by getdelim.\n\t *\n\t * Dying here is reasonable. It mirrors what xrealloc would do on\n\t * catastrophic memory failure. We skip the opportunity to free pack\n\t * memory and retry, but that's unlikely to help for a malloc small\n\t * enough to hold a single line of input, anyway.\n\t */\n\tif (errno == ENOMEM)\n\t\tdie(\"Out of memory, getdelim failed\");\n\nSo the function is returning -1 and leaving ENOMEM in errno on\nYaroslav's system.  \n\nI wonder if we are truly hitting out of memory, though.  The same\nsymptom could bee seen if getdelim() does not touch errno when it\nreturns -1, but some other system call earlier set it to ENOMEM,\nfor example.\n\nIf the same version of Git is recompiled there without HAVE_GETDELIM\ndefined, would it still die with out of memory (presumably inside\nthe call to strbuf_grow() in the strbuf_getwholeline() function)?\n"},{"id":"326080","messageId":"863176c5-5854-7a9b-ea38-d40621cead20@web.de","threadId":"46551","inReplyTo":"20170810144305.5d4lcx6oubkemsss@hopa.kiewit.dartmouth.edu","subject":"Re: fatal: Out of memory, getdelim failed under NFS mounts","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-08-10T19:44:22Z","receivedAt":"2017-08-10T19:44:41Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.08.2017 um 16:43 schrieb Yaroslav Halchenko:\n> On Thu, 10 Aug 2017, René Scharfe wrote:\n>> Am 09.08.2017 um 19:39 schrieb Yaroslav Halchenko:\n>>> More context (may be different issue(s)) could be found at\n>>> http://git-annex.branchable.com/forum/git-annex_add_out_of_memory_error/\n>>> but currently I am consistently reproducing it while running\n>>> git (1:2.11.0-3 debian stretch build) within debian stretch singularity\n>>> environment [1].\n> \n>>> External system is Centos 6.9, and git 1.7.1 (and installed in modules\n>>> 2.0.4) do not show similar buggy behavior.\n> \n>>> NFS mounted partitions are bind mounted inside the sinularity space and\n>>> when I try to do some git operations, I get that error inconsistently , e.g.\n> \n>>> \tyhalchen@discovery:/mnt/scratch/yoh/datalad$ git pull --ff-only origin master\n>>> \tfatal: Out of memory, getdelim failed\n>>> \terror: git://github.com/datalad/datalad did not send all necessary objects\n> \n>>> \tyhalchen@discovery:/mnt/scratch/yoh/datalad$ git pull --ff-only origin master\n>>> \tfatal: Out of memory, getdelim failed\n>>> \terror: git://github.com/datalad/datalad did not send all necessary objects\n> \n>>> \tyhalchen@discovery:/mnt/scratch/yoh/datalad$ git pull --ff-only origin master\n>>> \tFrom git://github.com/datalad/datalad\n>>> \t * branch              master     -> FETCH_HEAD\n>>> \tfatal: Out of memory, getdelim failed\n> \n>>> and some times it succeeds.  So it smells that some race condition\n>>> somewhere...?\n> \n>> I doubt the type of file system matters.\n> \n> So far it has been a very consistent indicator.  I did not manage to get\n> this error while performing the same operation under /tmp (bind to local\n> mounted drive), where it also feels going faster (again suggesting that\n> original issue is some kind of a race)\n\nWell, there have been bugs in getdelim() before, e.g.:\n\n  https://bugzilla.redhat.com/show_bug.cgi?id=601071\n  https://bugzilla.redhat.com/show_bug.cgi?id=1332917\n\ngit v2.5.0 was the first version to use it.   So if all else fails it may\nbe worth compiling git without HAVE_GETDELIM.\n\n>> The questions are: How much\n>> main memory do you have, what is git trying to cram into it, is there\n>> a way to reduce the memory footprint or do you need to add more RAM?\n>> ... reordered ...\n>> \"free\" and \"ulimit -a\" can help you find out how much memory you can\n>> use.\n> \n> I think those aren't the reason:\n> \n> yhalchen@discovery:/mnt/scratch/yoh/datalad$ free -h\n>                total        used        free      shared  buff/cache   available\n> Mem:           126G        2.5G         90G        652K         33G        123G\n> Swap:          127G        1.7M        127G\n\nIs all of that available to the git in the Singularity container or\nis that the memory size of the host and there's some kind of limit\nfor the guests?\n\n> yhalchen@discovery:/mnt/scratch/yoh/datalad$ ulimit\n> unlimited\n\nThat's just the maximum file size; memory-related limits are more\ninteresting for this case.  \"ulimit -a\" will show all limits.\n\n>>> any recommendations on how to pin point the \"offender\"? ;)\n>> Running \"GIT_TRACE=1 git pull --ff-only origin master\" would be a\n>> good start, I think, to find out which of the different activities\n>> that pull is doing causes the out-of-memory error.\n> \n> samples of bad, and then good runs (from eyeballing -- the same until\n> error message):\n> \n> yhalchen@discovery:/mnt/scratch/yoh$ cat git_trace_bad.log\n> 14:05:25.782270 git.c:371               trace: built-in: git 'pull' '--ff-only' 'origin' 'master'\n> 14:05:25.795036 run-command.c:350       trace: run_command: 'fetch' '--update-head-ok' 'origin' 'master'\n> 14:05:25.795332 exec_cmd.c:116          trace: exec: 'git' 'fetch' '--update-head-ok' 'origin' 'master'\n> 14:05:25.797212 git.c:371               trace: built-in: git 'fetch' '--update-head-ok' 'origin' 'master'\n> 14:05:25.904088 run-command.c:350       trace: run_command: 'rev-list' '--objects' '--stdin' '--not' '--all' '--quiet'\n> 14:05:26.085954 run-command.c:350       trace: run_command: 'index-pack' '--stdin' '--fix-thin' '--keep=fetch-pack 11652 on discovery.hpcc.dartmouth.edu' '--pack_header=2,103'\n> 14:05:26.086333 exec_cmd.c:116          trace: exec: 'git' 'index-pack' '--stdin' '--fix-thin' '--keep=fetch-pack 11652 on discovery.hpcc.dartmouth.edu' '--pack_header=2,103'\n> 14:05:26.088382 git.c:371               trace: built-in: git 'index-pack' '--stdin' '--fix-thin' '--keep=fetch-pack 11652 on discovery.hpcc.dartmouth.edu' '--pack_header=2,103'\n> 14:05:26.133326 run-command.c:350       trace: run_command: 'rev-list' '--objects' '--stdin' '--not' '--all' '--quiet'\n> 14:05:26.133688 exec_cmd.c:116          trace: exec: 'git' 'rev-list' '--objects' '--stdin' '--not' '--all' '--quiet'\n> 14:05:26.135493 git.c:371               trace: built-in: git 'rev-list' '--objects' '--stdin' '--not' '--all' '--quiet'\n> fatal: Out of memory, getdelim failed\n> error: git://github.com/datalad/datalad did not send all necessary objects\n\nThat rev-list call comes from connected::check_connected(); the error\nmessage from builtin/fetch.c::store_updated_refs(), which actually calls\ncheck_connected().  So you should be able to reproduce the issue just\nwith git fetch or with git rev-list.  The latter requires passing the\nright objects to the command, but perhaps reproduction is possible with\nguessed or arbitrary values.\n\nI don't know which files these commands access with getdelim() except\nfor the ones mentioned below, though.\n\n>> Also: What does \"wc -L .git/FETCH_HEAD .git/packed-refs\" report?\n> \n> \"varying\" and not consistent with causing an error (first trials, where I\n> did not cat .git/FETCH_HEAD kinda suggested differently):\n> \n> yhalchen@discovery:/mnt/scratch/yoh/datalad$ cat .git/FETCH_HEAD; wc -L .git/FETCH_HEAD .git/packed-refs; git pull --ff-only origin master\n> 1f90ef474ee200befea19ba77242fa44f16739f0                branch 'master' of git://github.com/datalad/datalad\n>   107 .git/FETCH_HEAD\n>    90 .git/packed-refs\n>   107 total\n\nThese line lengths are unlikely to exhaust the memory.  I was rather\nhoping for values in the range of billions due to some kind of freak\naccident or unconventional refs usage.\n\n> NB is there a diff which could be given regexes within a line to ignore\n> in diffs in so we could still retain original lines, i.e. answer to\n> https://stackoverflow.com/questions/15841223/diff-while-ignoring-patterns-within-a-line-but-not-the-entire-line\n> ?\n> \n> FWIW -- here is a diff (from good to bad run) of strace (with pid/address\n> info changed to stay the same for comparisons):\n> \n> @@ -3121,6 +3118,7 @@\n>   [pid YYYYY] close(3)                    = 0\n>   [pid YYYYY] brk(NULL)                   = 0xXXXXX\n>   [pid YYYYY] brk(0xXXXXX)         = 0xXXXXX\n> +[pid YYYYY] mmap(NULL, 1048576, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xXXXXX\n\nThat looks like malloc() increasing its pool by 1 MB...\n\n>   [pid YYYYY] open(\"/usr/lib/locale/locale-archive\", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)\n>   [pid YYYYY] open(\"/usr/share/locale/locale.alias\", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)\n>   [pid YYYYY] open(\"/usr/lib/locale/en_US/LC_MESSAGES\", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)\n> @@ -3144,11 +3142,9 @@\n>   [pid YYYYY] lstat(\".git/commondir\", 0xXXXXX) = -1 ENOENT (No such file or directory)\n>   [pid YYYYY] open(\".git/config\", O_RDONLY) = 3\n>   [pid YYYYY] fstat(3, {st_mode=S_IFREG|0644, st_size=257, ...}) = 0\n> -[pid YYYYY] mmap(NULL, 528384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xXXXXX\n>   [pid YYYYY] read(3, \"[core]\\n\\trepositoryformatversion \"..., 524288) = 257\n>   [pid YYYYY] read(3, \"\", 524288)         = 0\n>   [pid YYYYY] close(3)                    = 0\n> -[pid YYYYY] munmap(0xXXXXX, 528384) = 0\n\n... and in the working case it just gets 516 KB and releases it\nshortly afterwards...\n\n>   [pid YYYYY] stat(\".\", {st_mode=S_IFDIR|0755, st_size=907, ...}) = 0\n>   [pid YYYYY] getcwd(\"/mnt/scratch/yoh/datalad\", 129) = 25\n>   [pid YYYYY] chdir(\".\")                  = 0\n> @@ -3163,7 +3159,6 @@\n>   [pid YYYYY] access(\".git/config\", R_OK) = 0\n>   [pid YYYYY] open(\".git/config\", O_RDONLY) = 3\n>   [pid YYYYY] fstat(3, {st_mode=S_IFREG|0644, st_size=257, ...}) = 0\n> -[pid YYYYY] brk(0xXXXXX)         = 0xXXXXX\n>   [pid YYYYY] read(3, \"[core]\\n\\trepositoryformatversion \"..., 524288) = 257\n>   [pid YYYYY] read(3, \"\", 524288)         = 0\n>   [pid YYYYY] close(3)                    = 0\n> @@ -3180,245 +3175,48 @@\n>   [pid YYYYY] read(0, \"1f90ef474ee200befea19ba77242fa44\"..., 4096) = 82\n>   [pid YYYYY] open(\".git/refs/\", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 3\n>   [pid YYYYY] fstat(3, {st_mode=S_IFDIR|0755, st_size=70, ...}) = 0\n> -[pid YYYYY] brk(0xXXXXX)         = 0xXXXXX\n> +[pid YYYYY] mmap(NULL, 528384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xXXXXX\n>   [pid YYYYY] getdents(3, /* 5 entries */, 524288) = 136\n>   [pid YYYYY] stat(\".git/refs/heads\", {st_mode=S_IFDIR|0755, st_size=24, ...}) = 0\n>   [pid YYYYY] stat(\".git/refs/remotes\", {st_mode=S_IFDIR|0755, st_size=24, ...}) = 0\n>   [pid YYYYY] stat(\".git/refs/tags\", {st_mode=S_IFDIR|0755, st_size=47, ...}) = 0\n>   [pid YYYYY] getdents(3, /* 0 entries */, 524288) = 0\n> +[pid YYYYY] munmap(0xXXXXX, 528384) = 0\n\n... which is done in the non-working case as well, just a bit\nlater...\n\n>   [pid YYYYY] close(3)                    = 0\n>   [pid YYYYY] open(\".git/refs/bisect\", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = -1 ENOENT (No such file or directory)\n>   [pid YYYYY] open(\".git/packed-refs\", O_RDONLY) = 3\n>   [pid YYYYY] fstat(3, {st_mode=S_IFREG|0644, st_size=5042, ...}) = 0\n>   [pid YYYYY] fstat(3, {st_mode=S_IFREG|0644, st_size=5042, ...}) = 0\n> -[pid YYYYY] read(3, \"# pack-refs with: peeled fully-p\"..., 524288) = 5042\n\n... and it doesn't seem to reach the stage when the packed refs are read.\n\nSo the bad case has malloc() grab and never release one more megabyte.\nIs there are a limit for Singularity containers?  Can you increase the\none for yours by 1 MB? :)\n\nRené\n"},{"id":"326082","messageId":"20170810194438.n6cmljlorind2enu@hopa.kiewit.dartmouth.edu","threadId":"46551","inReplyTo":"xmqqzib72qvs.fsf@gitster.mtv.corp.google.com","subject":"Re: fatal: Out of memory, getdelim failed under NFS mounts","fromName":"Yaroslav Halchenko","fromEmail":"yoh@onerussian.com","sentAt":"2017-08-10T19:44:38Z","receivedAt":"2017-08-10T19:44:54Z","isPatch":false,"sender":{"key":"yoh@onerussian.com","avatar":"https://gravatar.com/avatar/8901b82415ae451a83aea49409708912726e53620e3ac92320bf1f86548d97e9?d=mp&s=160"},"body":"Thank you Junio\n\nOn Thu, 10 Aug 2017, Junio C Hamano wrote:\n> There is only one getdelim() call, which was introduced in v2.5.0\n> timeframe, and it is used like this:\n\n> \tr = getdelim(&sb->buf, &sb->alloc, term, fp);\n\n> \tif (r > 0) {\n> \t\tsb->len = r;\n> \t\treturn 0;\n> \t}\n> \tassert(r == -1);\n\n> \t/*\n> \t * Normally we would have called xrealloc, which will try to free\n> \t * memory and recover. But we have no way to tell getdelim() to do so.\n> \t * Worse, we cannot try to recover ENOMEM ourselves, because we have\n> \t * no idea how many bytes were read by getdelim.\n> \t *\n> \t * Dying here is reasonable. It mirrors what xrealloc would do on\n> \t * catastrophic memory failure. We skip the opportunity to free pack\n> \t * memory and retry, but that's unlikely to help for a malloc small\n> \t * enough to hold a single line of input, anyway.\n> \t */\n> \tif (errno == ENOMEM)\n> \t\tdie(\"Out of memory, getdelim failed\");\n\n> So the function is returning -1 and leaving ENOMEM in errno on\n> Yaroslav's system.  \n\n> I wonder if we are truly hitting out of memory, though.  The same\n> symptom could bee seen if getdelim() does not touch errno when it\n> returns -1, but some other system call earlier set it to ENOMEM,\n> for example.\n\n> If the same version of Git is recompiled there without HAVE_GETDELIM\n> defined, would it still die with out of memory (presumably inside\n> the call to strbuf_grow() in the strbuf_getwholeline() function)?\n\nwill check now...  for my own education (rotten by Python) -- how\ndo you know which syscall set errno to be analyzed at this specific\npoint?  may be it was already set to ENOMEM before entry to this\nfunction?\n\n-- \nYaroslav O. Halchenko\nCenter for Open Neuroscience     http://centerforopenneuroscience.org\nDartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755\nPhone: +1 (603) 646-9834                       Fax: +1 (603) 646-1419\nWWW:   http://www.linkedin.com/in/yarik        \n"},{"id":"326084","messageId":"6d7b0d30-48ea-f79f-78cd-088557ea06ac@web.de","threadId":"46551","inReplyTo":"xmqqzib72qvs.fsf@gitster.mtv.corp.google.com","subject":"Re: fatal: Out of memory, getdelim failed under NFS mounts","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-08-10T19:58:37Z","receivedAt":"2017-08-10T19:58:57Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.08.2017 um 20:56 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n> \n>> I doubt the type of file system matters.  The questions are: How much\n>> main memory do you have, what is git trying to cram into it, is there\n>> a way to reduce the memory footprint or do you need to add more RAM?\n>>\n>>> any recommendations on how to pin point the \"offender\"? ;)\n>> Running \"GIT_TRACE=1 git pull --ff-only origin master\" would be a\n>> good start, I think, to find out which of the different activities\n>> that pull is doing causes the out-of-memory error.\n>>\n>> \"free\" and \"ulimit -a\" can help you find out how much memory you can\n>> use.\n>>\n>> Also: What does \"wc -L .git/FETCH_HEAD .git/packed-refs\" report?\n>> getdelim() is used mostly to read lines from files like these and in\n>> the admittedly unlikely case that they are *really* long such an\n>> error would be expected.\n> \n> There is only one getdelim() call, which was introduced in v2.5.0\n> timeframe, and it is used like this:\n> \n> \tr = getdelim(&sb->buf, &sb->alloc, term, fp);\n> \n> \tif (r > 0) {\n> \t\tsb->len = r;\n> \t\treturn 0;\n> \t}\n> \tassert(r == -1);\n> \n> \t/*\n> \t * Normally we would have called xrealloc, which will try to free\n> \t * memory and recover. But we have no way to tell getdelim() to do so.\n> \t * Worse, we cannot try to recover ENOMEM ourselves, because we have\n> \t * no idea how many bytes were read by getdelim.\n> \t *\n> \t * Dying here is reasonable. It mirrors what xrealloc would do on\n> \t * catastrophic memory failure. We skip the opportunity to free pack\n> \t * memory and retry, but that's unlikely to help for a malloc small\n> \t * enough to hold a single line of input, anyway.\n> \t */\n> \tif (errno == ENOMEM)\n> \t\tdie(\"Out of memory, getdelim failed\");\n> \n> So the function is returning -1 and leaving ENOMEM in errno on\n> Yaroslav's system.\n> \n> I wonder if we are truly hitting out of memory, though.  The same\n> symptom could bee seen if getdelim() does not touch errno when it\n> returns -1, but some other system call earlier set it to ENOMEM,\n> for example.\n\nThat can happen when the end of a file is reached; getdelim() returns\n-1 and leaves errno unchanged.  So we need to set errno to 0 just\nbefore that call.\n\nStill *some* function must have run into a memory shortage in that\nscenario, so adding/assigning more should help, right?\n\n> If the same version of Git is recompiled there without HAVE_GETDELIM\n> defined, would it still die with out of memory (presumably inside\n> the call to strbuf_grow() in the strbuf_getwholeline() function)?\n\nIt's worth a try, if possible.\n\nRené\n"},{"id":"326086","messageId":"20170810200502.rutab4z3ft7gcpjz@sigill.intra.peff.net","threadId":"46551","inReplyTo":"6d7b0d30-48ea-f79f-78cd-088557ea06ac@web.de","subject":"Re: fatal: Out of memory, getdelim failed under NFS mounts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-10T20:05:02Z","receivedAt":"2017-08-10T20:05:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 10, 2017 at 09:58:37PM +0200, René Scharfe wrote:\n\n> > So the function is returning -1 and leaving ENOMEM in errno on\n> > Yaroslav's system.\n> > \n> > I wonder if we are truly hitting out of memory, though.  The same\n> > symptom could bee seen if getdelim() does not touch errno when it\n> > returns -1, but some other system call earlier set it to ENOMEM,\n> > for example.\n> \n> That can happen when the end of a file is reached; getdelim() returns\n> -1 and leaves errno unchanged.  So we need to set errno to 0 just\n> before that call.\n\nGood catch. That's a bug in my original conversoin of\nstrbuf_getwholeline().\n\n-Peff\n"},{"id":"326090","messageId":"20170810202942.s232yr5stjxmmz4t@hopa.kiewit.dartmouth.edu","threadId":"46551","inReplyTo":"20170810200502.rutab4z3ft7gcpjz@sigill.intra.peff.net","subject":"Re: fatal: Out of memory, getdelim failed under NFS mounts","fromName":"Yaroslav Halchenko","fromEmail":"yoh@onerussian.com","sentAt":"2017-08-10T20:29:42Z","receivedAt":"2017-08-10T20:29:54Z","isPatch":false,"sender":{"key":"yoh@onerussian.com","avatar":"https://gravatar.com/avatar/8901b82415ae451a83aea49409708912726e53620e3ac92320bf1f86548d97e9?d=mp&s=160"},"body":"\nOn Thu, 10 Aug 2017, Jeff King wrote:\n\n> On Thu, Aug 10, 2017 at 09:58:37PM +0200, René Scharfe wrote:\n\n> > > So the function is returning -1 and leaving ENOMEM in errno on\n> > > Yaroslav's system.\n\n> > > I wonder if we are truly hitting out of memory, though.  The same\n> > > symptom could bee seen if getdelim() does not touch errno when it\n> > > returns -1, but some other system call earlier set it to ENOMEM,\n> > > for example.\n\n> > That can happen when the end of a file is reached; getdelim() returns\n> > -1 and leaves errno unchanged.  So we need to set errno to 0 just\n> > before that call.\n\n> Good catch. That's a bug in my original conversoin of\n> strbuf_getwholeline().\n\nI think this did it!  at least on this simple test... yet to test a bit\nmore in those scenarios we ran into it before while testing git-annex.\n\ncommit 36ef5e3ad2c187d3be664c33dbc8c06e59bceaf4 (HEAD -> bf-seterrno0)\nAuthor: Yaroslav O. Halchenko <yhalchen@smaug.cns.dartmouth.edu>\nDate:   Thu Aug 10 20:26:47 2017 +0000\n\n    BF: set errno to 0 before getdelim call to get unbiased assesment later\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 89d22e3b0..323c49ceb 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -476,6 +476,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n        /* Translate slopbuf to NULL, as we cannot call realloc on it */\n        if (!sb->alloc)\n                sb->buf = NULL;\n+       errno = 0;\n        r = getdelim(&sb->buf, &sb->alloc, term, fp);\n \n        if (r > 0) {\n\n-- \nYaroslav O. Halchenko\nCenter for Open Neuroscience     http://centerforopenneuroscience.org\nDartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755\nPhone: +1 (603) 646-9834                       Fax: +1 (603) 646-1419\nWWW:   http://www.linkedin.com/in/yarik        \n"},{"id":"326095","messageId":"cd49ce13-db87-89c1-77e7-998fdb9442c3@web.de","threadId":"46551","inReplyTo":"20170810200502.rutab4z3ft7gcpjz@sigill.intra.peff.net","subject":"[PATCH] strbuf: clear errno before calling getdelim(3)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-08-10T20:56:40Z","receivedAt":"2017-08-10T20:57:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"getdelim(3) returns -1 at the end of the file and if it encounters an\nerror, but sets errno only in the latter case.  Set errno to zero before\ncalling it to avoid misdiagnosing an out-of-memory condition due to a\nleft-over value from some other function call.\n\nReported-by: Yaroslav Halchenko <yoh@onerussian.com>\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\nDo we need to save and restore the original value of errno?  I doubt it,\nbut didn't think deeply about it, yet.\n\n strbuf.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 89d22e3b09..323c49ceb3 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -476,6 +476,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n \t/* Translate slopbuf to NULL, as we cannot call realloc on it */\n \tif (!sb->alloc)\n \t\tsb->buf = NULL;\n+\terrno = 0;\n \tr = getdelim(&sb->buf, &sb->alloc, term, fp);\n \n \tif (r > 0) {\n-- \n2.14.0\n"},{"id":"326096","messageId":"20170810210246.p4vp2ibahgsii7fc@sigill.intra.peff.net","threadId":"46551","inReplyTo":"cd49ce13-db87-89c1-77e7-998fdb9442c3@web.de","subject":"Re: [PATCH] strbuf: clear errno before calling getdelim(3)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-10T21:02:46Z","receivedAt":"2017-08-10T21:02:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 10, 2017 at 10:56:40PM +0200, René Scharfe wrote:\n\n> getdelim(3) returns -1 at the end of the file and if it encounters an\n> error, but sets errno only in the latter case.  Set errno to zero before\n> calling it to avoid misdiagnosing an out-of-memory condition due to a\n> left-over value from some other function call.\n\nLooks good to me.\n\n> Do we need to save and restore the original value of errno?  I doubt it,\n> but didn't think deeply about it, yet.\n\nI'd say no. Anybody depending on strbuf_getwholeline() is clearly\nalready wrong in the error case. And in general I think we assume that\nsyscalls can clear errno on success if they choose to (this isn't a\nsyscall, but obviously it is calling some).\n\n-Peff\n"},{"id":"326104","messageId":"20170810213555.fejnalargwwlws6k@hopa.kiewit.dartmouth.edu","threadId":"46551","inReplyTo":"20170810210246.p4vp2ibahgsii7fc@sigill.intra.peff.net","subject":"Re: [PATCH] strbuf: clear errno before calling getdelim(3)","fromName":"Yaroslav Halchenko","fromEmail":"yoh@onerussian.com","sentAt":"2017-08-10T21:35:55Z","receivedAt":"2017-08-10T21:36:07Z","isPatch":true,"sender":{"key":"yoh@onerussian.com","avatar":"https://gravatar.com/avatar/8901b82415ae451a83aea49409708912726e53620e3ac92320bf1f86548d97e9?d=mp&s=160"},"body":"\nOn Thu, 10 Aug 2017, Jeff King wrote:\n\n> On Thu, Aug 10, 2017 at 10:56:40PM +0200, René Scharfe wrote:\n\n> > getdelim(3) returns -1 at the end of the file and if it encounters an\n> > error, but sets errno only in the latter case.  Set errno to zero before\n> > calling it to avoid misdiagnosing an out-of-memory condition due to a\n> > left-over value from some other function call.\n\n> Looks good to me.\n\n> > Do we need to save and restore the original value of errno?  I doubt it,\n> > but didn't think deeply about it, yet.\n\n> I'd say no. Anybody depending on strbuf_getwholeline() is clearly\n> already wrong in the error case. And in general I think we assume that\n> syscalls can clear errno on success if they choose to (this isn't a\n> syscall, but obviously it is calling some).\n\nShouldn't ideally errno being reset to 0 upon check of the syscall\nsuccessful run right after that syscall?  (I also see some spots within\ngit code where it sets errno to ENOMEM)\n\n\n-- \nYaroslav O. Halchenko\nCenter for Open Neuroscience     http://centerforopenneuroscience.org\nDartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755\nPhone: +1 (603) 646-9834                       Fax: +1 (603) 646-1419\nWWW:   http://www.linkedin.com/in/yarik        \n"},{"id":"326105","messageId":"xmqq378z2j8z.fsf@gitster.mtv.corp.google.com","threadId":"46551","inReplyTo":"cd49ce13-db87-89c1-77e7-998fdb9442c3@web.de","subject":"Re: [PATCH] strbuf: clear errno before calling getdelim(3)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-10T21:41:32Z","receivedAt":"2017-08-10T21:41:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> getdelim(3) returns -1 at the end of the file and if it encounters an\n> error, but sets errno only in the latter case.  Set errno to zero before\n> calling it to avoid misdiagnosing an out-of-memory condition due to a\n> left-over value from some other function call.\n>\n> Reported-by: Yaroslav Halchenko <yoh@onerussian.com>\n> Suggested-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n\nHeh.  I mumble something vague then people more capable than me jump\nin to take it to the conclusion, and still I get the credit.  I wish\nall the debugging sessions were this easy ;-)\n\n> ---\n> Do we need to save and restore the original value of errno?  I doubt it,\n> but didn't think deeply about it, yet.\n\nWe probably don't need to---a caller who knows it got an error\nbefore calling this function and wants to use errno after doing so\nshould be stashing it away; after all, this function will clobber\nerrno when any of the library calls it makes fails and this is on\nthe I/O codepath, so anything can go wrong.\n\nThanks.\n\n>\n>  strbuf.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index 89d22e3b09..323c49ceb3 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -476,6 +476,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n>  \t/* Translate slopbuf to NULL, as we cannot call realloc on it */\n>  \tif (!sb->alloc)\n>  \t\tsb->buf = NULL;\n> +\terrno = 0;\n>  \tr = getdelim(&sb->buf, &sb->alloc, term, fp);\n>  \n>  \tif (r > 0) {\n"},{"id":"326134","messageId":"20170811075059.nn6lru7uy6s6vpza@ruderich.org","threadId":"46551","inReplyTo":"cd49ce13-db87-89c1-77e7-998fdb9442c3@web.de","subject":"Re: [PATCH] strbuf: clear errno before calling getdelim(3)","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2017-08-11T07:50:59Z","receivedAt":"2017-08-11T07:56:19Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Thu, Aug 10, 2017 at 10:56:40PM +0200, René Scharfe wrote:\n> getdelim(3) returns -1 at the end of the file and if it encounters an\n> error, but sets errno only in the latter case.  Set errno to zero before\n> calling it to avoid misdiagnosing an out-of-memory condition due to a\n> left-over value from some other function call.\n\ngetdelim(3p) doesn't explicitly forbid changing the errno on EOF:\n\n    If no characters were read, and the end-of-file indicator for\n    the stream is set, or if the stream is at end-of-file, the\n    end-of-file indicator for the stream shall be set and the\n    function shall return −1. If an error occurs, the error\n    indicator for the stream shall be set, and the function shall\n    return −1 and set errno to indicate the error.\n\nSo a valid implementation could still set errno on EOF and also\non another error (where it's required to set errno).\n\nI don't think that it matters in practice, but the \"most\" correct\nway to handle this would be to check if feof(3) is true to check\nfor the non-errno case.\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"326136","messageId":"e8e7f028-6e23-368c-484f-9f069bae5dc8@web.de","threadId":"46551","inReplyTo":"20170811075059.nn6lru7uy6s6vpza@ruderich.org","subject":"Re: [PATCH] strbuf: clear errno before calling getdelim(3)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-08-11T08:52:51Z","receivedAt":"2017-08-11T08:53:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.08.2017 um 09:50 schrieb Simon Ruderich:\n> On Thu, Aug 10, 2017 at 10:56:40PM +0200, René Scharfe wrote:\n>> getdelim(3) returns -1 at the end of the file and if it encounters an\n>> error, but sets errno only in the latter case.  Set errno to zero before\n>> calling it to avoid misdiagnosing an out-of-memory condition due to a\n>> left-over value from some other function call.\n> \n> getdelim(3p) doesn't explicitly forbid changing the errno on EOF:\n> \n>      If no characters were read, and the end-of-file indicator for\n>      the stream is set, or if the stream is at end-of-file, the\n>      end-of-file indicator for the stream shall be set and the\n>      function shall return −1. If an error occurs, the error\n>      indicator for the stream shall be set, and the function shall\n>      return −1 and set errno to indicate the error.\n> \n> So a valid implementation could still set errno on EOF and also\n> on another error (where it's required to set errno).\n\nTrue, especially the part that other errors are possible.  But we can't\nrely on errno being set on EOF because leaving it unchanged is allowed\nas well in that\n case.\n\n> I don't think that it matters in practice, but the \"most\" correct\n> way to handle this would be to check if feof(3) is true to check\n> for the non-errno case.\n\nOnly if errors at EOF are guaranteed to be impossible.  Imagine\ngetdelim being able to read to the end and then failing to get memory\nfor the final NUL.  Other scenarios may be possible.\n\nRené\n"},{"id":"326210","messageId":"20170812100252.bhbgg5jjlom7dfl2@ruderich.org","threadId":"46551","inReplyTo":"e8e7f028-6e23-368c-484f-9f069bae5dc8@web.de","subject":"Re: [PATCH] strbuf: clear errno before calling getdelim(3)","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2017-08-12T10:02:52Z","receivedAt":"2017-08-12T10:02:59Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Fri, Aug 11, 2017 at 10:52:51AM +0200, René Scharfe wrote:\n> Am 11.08.2017 um 09:50 schrieb Simon Ruderich:\n>> On Thu, Aug 10, 2017 at 10:56:40PM +0200, René Scharfe wrote:\n>>> getdelim(3) returns -1 at the end of the file and if it encounters an\n>>> error, but sets errno only in the latter case.  Set errno to zero before\n>>> calling it to avoid misdiagnosing an out-of-memory condition due to a\n>>> left-over value from some other function call.\n>>\n>> getdelim(3p) doesn't explicitly forbid changing the errno on EOF:\n>>\n>>      If no characters were read, and the end-of-file indicator for\n>>      the stream is set, or if the stream is at end-of-file, the\n>>      end-of-file indicator for the stream shall be set and the\n>>      function shall return −1. If an error occurs, the error\n>>      indicator for the stream shall be set, and the function shall\n>>      return −1 and set errno to indicate the error.\n>\n> [snip]\n>\n>> I don't think that it matters in practice, but the \"most\" correct\n>> way to handle this would be to check if feof(3) is true to check\n>> for the non-errno case.\n>\n> Only if errors at EOF are guaranteed to be impossible.  Imagine\n> getdelim being able to read to the end and then failing to get memory\n> for the final NUL.  Other scenarios may be possible.\n\nGood point. Instead of feof(3), checking ferror(3) should handle\nthat properly. It's guaranteed to be set by getdelim for any\nerror.\n\nFor example like this (as replacement for the original patch):\n\ndiff --git i/strbuf.c w/strbuf.c\nindex 89d22e3b0..831be21ce 100644\n--- i/strbuf.c\n+++ w/strbuf.c\n@@ -495,7 +495,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n \t * memory and retry, but that's unlikely to help for a malloc small\n \t * enough to hold a single line of input, anyway.\n \t */\n-\tif (errno == ENOMEM)\n+\tif (ferror(fp) && errno == ENOMEM)\n \t\tdie(\"Out of memory, getdelim failed\");\n \n \t/*\n\nAn edge case is if the error indicator was already set before\ncalling getdelim() and the errno was already set to ENOMEM. This\ncould be fixed by checking ferror() before calling getdelim, but\nI'm not sure if that's necessary:\n\ndiff --git i/strbuf.c w/strbuf.c\nindex 89d22e3b0..4d394bb91 100644\n--- i/strbuf.c\n+++ w/strbuf.c\n@@ -468,7 +468,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n {\n \tssize_t r;\n \n-\tif (feof(fp))\n+\tif (feof(fp) || ferror(fp))\n \t\treturn EOF;\n \n \tstrbuf_reset(sb);\n\nRegards\nSimon\n-- \n+ Privatsphäre ist notwendig\n+ Ich verwende GnuPG http://gnupg.org\n+ Öffentlicher Schlüssel: 0x92FEFDB7E44C32F9\n"},{"id":"326211","messageId":"510f566b-3aee-98fd-9663-d97c3dcfeb96@web.de","threadId":"46551","inReplyTo":"20170812100252.bhbgg5jjlom7dfl2@ruderich.org","subject":"Re: [PATCH] strbuf: clear errno before calling getdelim(3)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-08-12T11:57:06Z","receivedAt":"2017-08-12T11:57:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.08.2017 um 12:02 schrieb Simon Ruderich:\n> On Fri, Aug 11, 2017 at 10:52:51AM +0200, René Scharfe wrote:\n>> Am 11.08.2017 um 09:50 schrieb Simon Ruderich:\n>>> On Thu, Aug 10, 2017 at 10:56:40PM +0200, René Scharfe wrote:\n>>>> getdelim(3) returns -1 at the end of the file and if it encounters an\n>>>> error, but sets errno only in the latter case.  Set errno to zero before\n>>>> calling it to avoid misdiagnosing an out-of-memory condition due to a\n>>>> left-over value from some other function call.\n>>>\n>>> getdelim(3p) doesn't explicitly forbid changing the errno on EOF:\n>>>\n>>>       If no characters were read, and the end-of-file indicator for\n>>>       the stream is set, or if the stream is at end-of-file, the\n>>>       end-of-file indicator for the stream shall be set and the\n>>>       function shall return −1. If an error occurs, the error\n>>>       indicator for the stream shall be set, and the function shall\n>>>       return −1 and set errno to indicate the error.\n>>\n>> [snip]\n>>\n>>> I don't think that it matters in practice, but the \"most\" correct\n>>> way to handle this would be to check if feof(3) is true to check\n>>> for the non-errno case.\n>>\n>> Only if errors at EOF are guaranteed to be impossible.  Imagine\n>> getdelim being able to read to the end and then failing to get memory\n>> for the final NUL.  Other scenarios may be possible.\n> \n> Good point. Instead of feof(3), checking ferror(3) should handle\n> that properly. It's guaranteed to be set by getdelim for any\n> error.\n\nOnly if we have a POSIXly correct implementation of getdelim(3). On\nLinux its manpage doesn't mention the error indicator, but says that\nthe function started out as a GNU extension before becoming\nstandardized, so I'm not sure we can depend on that everywhere.\n\nBut we probably want to check for other errors.  They look unlikely\nenough that we may get away with something like this:\n\n\t-\tif (errno == ENOMEM)\n\t-\t\tdie(\"Out of memory, getdelim failed\");\n\t+\tif (errno || ferror(fp))\n\t+\t\tdie_errno(_(\"getdelim failed\"));\n\nNB: The other errors are EINVAL (input pointers are NULL or the\nstream is invalid) and EOVERFLOW (read more than fits into ssize_t)\naccording to POSIX and the Linux manpage.\n\nUpdate: Just noticed that on the BSDs getdelim(3) doesn't set errno\nto ENOMEM on allocation failure, but does set the error indicator.\nThat might be an oversight on their part, but that means we\ncertainly *need* to check ferror().  And an unchanged errno can\nmean ENOMEM there.  Ugh..\n\n> An edge case is if the error indicator was already set before\n> calling getdelim() and the errno was already set to ENOMEM. This\n> could be fixed by checking ferror() before calling getdelim, but\n> I'm not sure if that's necessary:\n\nTreating an error as end-of-file is not a good idea.  An invalid\nstream should not be passed to strbuf_getwholeline() in the first\nplace.  We could call die() or BUG(), but I think we may leave\nthat check to getdelim(3), unless we learn about an implementation\nthat is unprepared to reject streams with the error indicator set.\n\nRené\n"},{"id":"326212","messageId":"a6fe4020-4e3d-0561-db70-2eb87e0680c7@web.de","threadId":"46551","inReplyTo":"510f566b-3aee-98fd-9663-d97c3dcfeb96@web.de","subject":"Re: [PATCH] strbuf: clear errno before calling getdelim(3)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-08-12T12:21:12Z","receivedAt":"2017-08-12T12:21:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.08.2017 um 13:57 schrieb René Scharfe:\n> Update: Just noticed that on the BSDs getdelim(3) doesn't set errno\n> to ENOMEM on allocation failure, but does set the error indicator.\n> That might be an oversight on their part, but that means we\n> certainly *need* to check ferror().  And an unchanged errno can\n> mean ENOMEM there.  Ugh..\n\nI take that back.  The memory allocation function called by getdelim\nwill of course set errno on failure.  I got fooled by the manpages,\nwhich don't mention ENOMEM, e.g.: https://man.openbsd.org/getdelim.\n\nRené\n"},{"id":"326229","messageId":"20170813043237.ndcm77i43ivepo7b@sigill.intra.peff.net","threadId":"46551","inReplyTo":"510f566b-3aee-98fd-9663-d97c3dcfeb96@web.de","subject":"Re: [PATCH] strbuf: clear errno before calling getdelim(3)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-13T04:32:37Z","receivedAt":"2017-08-13T04:32:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 12, 2017 at 01:57:06PM +0200, René Scharfe wrote:\n\n> But we probably want to check for other errors.  They look unlikely\n> enough that we may get away with something like this:\n> \n> \t-\tif (errno == ENOMEM)\n> \t-\t\tdie(\"Out of memory, getdelim failed\");\n> \t+\tif (errno || ferror(fp))\n> \t+\t\tdie_errno(_(\"getdelim failed\"));\n> \n> NB: The other errors are EINVAL (input pointers are NULL or the\n> stream is invalid) and EOVERFLOW (read more than fits into ssize_t)\n> according to POSIX and the Linux manpage.\n\nCan't we also get any of the errors that fgetc() would return. I.e., any\nnormal read errors? We should return EOF on those, not die (and the\ncaller can check ferror()).\n\n-Peff\n"},{"id":"326234","messageId":"4bf18555-f8ea-a3f6-5204-b02bfd8f1ff9@web.de","threadId":"46551","inReplyTo":"20170813043237.ndcm77i43ivepo7b@sigill.intra.peff.net","subject":"Re: [PATCH] strbuf: clear errno before calling getdelim(3)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-08-13T05:51:34Z","receivedAt":"2017-08-13T05:52:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.08.2017 um 06:32 schrieb Jeff King:\n> On Sat, Aug 12, 2017 at 01:57:06PM +0200, René Scharfe wrote:\n> \n>> But we probably want to check for other errors.  They look unlikely\n>> enough that we may get away with something like this:\n>>\n>> \t-\tif (errno == ENOMEM)\n>> \t-\t\tdie(\"Out of memory, getdelim failed\");\n>> \t+\tif (errno || ferror(fp))\n>> \t+\t\tdie_errno(_(\"getdelim failed\"));\n>>\n>> NB: The other errors are EINVAL (input pointers are NULL or the\n>> stream is invalid) and EOVERFLOW (read more than fits into ssize_t)\n>> according to POSIX and the Linux manpage.\n> \n> Can't we also get any of the errors that fgetc() would return. I.e., any\n> normal read errors? We should return EOF on those, not die (and the\n> caller can check ferror()).\n\nYes, we can get those as well, and leaving error checking to the caller\nis a flexible way to handle them.  \n\nMany of the existing callers don't seem to be bother, though.\n\nRené\n"},{"id":"326235","messageId":"20170813055853.vrrfot3ijrpjk2rv@sigill.intra.peff.net","threadId":"46551","inReplyTo":"4bf18555-f8ea-a3f6-5204-b02bfd8f1ff9@web.de","subject":"Re: [PATCH] strbuf: clear errno before calling getdelim(3)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-13T05:58:53Z","receivedAt":"2017-08-13T05:59:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 13, 2017 at 07:51:34AM +0200, René Scharfe wrote:\n\n> Am 13.08.2017 um 06:32 schrieb Jeff King:\n> > On Sat, Aug 12, 2017 at 01:57:06PM +0200, René Scharfe wrote:\n> > \n> >> But we probably want to check for other errors.  They look unlikely\n> >> enough that we may get away with something like this:\n> >>\n> >> \t-\tif (errno == ENOMEM)\n> >> \t-\t\tdie(\"Out of memory, getdelim failed\");\n> >> \t+\tif (errno || ferror(fp))\n> >> \t+\t\tdie_errno(_(\"getdelim failed\"));\n> >>\n> >> NB: The other errors are EINVAL (input pointers are NULL or the\n> >> stream is invalid) and EOVERFLOW (read more than fits into ssize_t)\n> >> according to POSIX and the Linux manpage.\n> > \n> > Can't we also get any of the errors that fgetc() would return. I.e., any\n> > normal read errors? We should return EOF on those, not die (and the\n> > caller can check ferror()).\n> \n> Yes, we can get those as well, and leaving error checking to the caller\n> is a flexible way to handle them.  \n> \n> Many of the existing callers don't seem to be bother, though.\n\nYes, but keep in mind that we need to match the non-getdelim()\nimplementation here (which itself tries to behave like more or less like\na dynamic fgets, including the eof/error handling). Because the callers\ndon't know which one they're getting.\n\n-Peff\n"}]}