{"thread":{"id":"46742","subject":"Bug: git branch --unset-upstream command can nuke config when disk is full.","startedAt":"2017-09-13T11:59:23Z","lastAt":"2017-09-15T15:15:22Z","messageCount":46,"participants":["demerphq","Jeff King","Jonathan Nieder","Junio C Hamano","Ramsay Jones"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"327924","messageId":"CANgJU+Wj_sN8Px6rweHMw_iAX8UA3mW9x4NVCctpUCs8_3ytLw@mail.gmail.com","threadId":"46742","inReplyTo":null,"subject":"Bug: git branch --unset-upstream command can nuke config when disk is full.","fromName":"demerphq","fromEmail":"demerphq@gmail.com","sentAt":"2017-09-13T11:59:17Z","receivedAt":"2017-09-13T11:59:23Z","isPatch":false,"sender":{"key":"demerphq@gmail.com","avatar":null},"body":"After being away for a while I saw the following message in one of my git repos:\n\n$ git status\nOn branch yves/xxx\nYour branch is based on 'origin/yves/xxx', but the upstream is gone.\n (use \"git branch --unset-upstream\" to fixup)\n\nnothing to commit, working tree clean\n$ git branch --unset-upstream\nfatal: could not unset 'branch.yves/simple_projection.merge'\n\nAt this point my .git/config file was empty, and all of my config was lost.\n\nI assume that things that rewrite .git/config do not check for a\nsuccessful write before deleting the old version of the file.\n\nThis was git version 2.14.1\n\nYves\n\n\n\n-- \nperl -Mre=debug -e \"/just|another|perl|hacker/\"\n"},{"id":"327928","messageId":"20170913123420.die3thfcpaweakce@sigill.intra.peff.net","threadId":"46742","inReplyTo":"CANgJU+Wj_sN8Px6rweHMw_iAX8UA3mW9x4NVCctpUCs8_3ytLw@mail.gmail.com","subject":"Re: Bug: git branch --unset-upstream command can nuke config when disk is full.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T12:34:21Z","receivedAt":"2017-09-13T12:34:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2017 at 01:59:17PM +0200, demerphq wrote:\n\n> After being away for a while I saw the following message in one of my git repos:\n> \n> $ git status\n> On branch yves/xxx\n> Your branch is based on 'origin/yves/xxx', but the upstream is gone.\n> (use \"git branch --unset-upstream\" to fixup)\n> \n> nothing to commit, working tree clean\n> $ git branch --unset-upstream\n> fatal: could not unset 'branch.yves/simple_projection.merge'\n\nHrm. I wonder what caused this failure. The error would be in\ngit_config_set_multivar_in_file_gently(). Most errors there produce\nanother error message before hitting the die(). In fact, the only case I\nsee where it would not produce another message is if it found nothing to\nunset (but in that case, \"branch\" would never have called the function\nin the first place).\n\n> At this point my .git/config file was empty, and all of my config was lost.\n> \n> I assume that things that rewrite .git/config do not check for a\n> successful write before deleting the old version of the file.\n\nNo, it writes the new content to \"config.lock\" and then renames it into\nplace. All of the write() calls to the temporary file are checked. The\nold data is copied over after having been ready by mmap (which is also\nerror-checked).\n\nGiven that your output is consistent with it failing to find the key,\nand that the result is an empty file, it sounds like somehow the mmap'd\ninput appeared empty (but neither open nor fstat nor mmap returned an\nerror). You're not on any kind of exotic filesystem, are you?\n\n-Peff\n"},{"id":"327935","messageId":"CANgJU+U3Vzmmag9fpFGf7sx4Sq4S9qjLc5bnR4e+U0C3aaQXJA@mail.gmail.com","threadId":"46742","inReplyTo":"20170913123420.die3thfcpaweakce@sigill.intra.peff.net","subject":"Re: Bug: git branch --unset-upstream command can nuke config when disk is full.","fromName":"demerphq","fromEmail":"demerphq@gmail.com","sentAt":"2017-09-13T13:38:52Z","receivedAt":"2017-09-13T13:39:05Z","isPatch":false,"sender":{"key":"demerphq@gmail.com","avatar":null},"body":"On 13 September 2017 at 14:34, Jeff King wrote:\n> On Wed, Sep 13, 2017 at 01:59:17PM +0200, demerphq wrote:\n>\n>> After being away for a while I saw the following message in one of my git repos:\n>>\n>> $ git status\n>> On branch yves/xxx\n>> Your branch is based on 'origin/yves/xxx', but the upstream is gone.\n>> (use \"git branch --unset-upstream\" to fixup)\n>>\n>> nothing to commit, working tree clean\n>> $ git branch --unset-upstream\n>> fatal: could not unset 'branch.yves/simple_projection.merge'\n>\n> Hrm. I wonder what caused this failure. The error would be in\n> git_config_set_multivar_in_file_gently(). Most errors there produce\n> another error message before hitting the die(). In fact, the only case I\n> see where it would not produce another message is if it found nothing to\n> unset (but in that case, \"branch\" would never have called the function\n> in the first place).\n\nI just double checked the terminal history and this is all i saw:\n\n$ git status\nOn branch yves/xxx\nYour branch is based on 'origin/yves/xxx', but the upstream is gone.\n (use \"git branch --unset-upstream\" to fixup)\n\nnothing to commit, working tree clean\n$ git branch --unset-upstream\nfatal: could not unset 'branch.yves/xxx.merge'\n$ git status\nOn branch yves/xxx\nnothing to commit, working tree clean\n$ git fetch\nfatal: No remote repository specified. Please, specify either a URL or a\nremote name from which new revisions should be fetched.\n\n>> At this point my .git/config file was empty, and all of my config was lost.\n>>\n>> I assume that things that rewrite .git/config do not check for a\n>> successful write before deleting the old version of the file.\n>\n> No, it writes the new content to \"config.lock\" and then renames it into\n> place.\n> All of the write() calls to the temporary file are checked.\n\nI was going to say that perhaps the write was not checked... But if\nyou are confident they are then...\n\n>The\n> old data is copied over after having been ready by mmap (which is also\n> error-checked).\n>\n> Given that your output is consistent with it failing to find the key,\n> and that the result is an empty file, it sounds like somehow the mmap'd\n> input appeared empty (but neither open nor fstat nor mmap returned an\n> error). You're not on any kind of exotic filesystem, are you?\n\nI don't think so, but I don't know. Is there a command I can run to check?\n\nBTW, with a bit of faffing I can probably recreate this problem.\nShould I try? Is there something I could do during recreation that\nwould help?\n\ncheers,\nYves\n\n-- \nperl -Mre=debug -e \"/just|another|perl|hacker/\"\n"},{"id":"327938","messageId":"20170913141739.mkmiaud2eemgnbbh@sigill.intra.peff.net","threadId":"46742","inReplyTo":"CANgJU+U3Vzmmag9fpFGf7sx4Sq4S9qjLc5bnR4e+U0C3aaQXJA@mail.gmail.com","subject":"Re: Bug: git branch --unset-upstream command can nuke config when disk is full.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T14:17:40Z","receivedAt":"2017-09-13T14:17:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2017 at 03:38:52PM +0200, demerphq wrote:\n\n> I just double checked the terminal history and this is all i saw:\n> \n> $ git status\n> On branch yves/xxx\n> Your branch is based on 'origin/yves/xxx', but the upstream is gone.\n> (use \"git branch --unset-upstream\" to fixup)\n> \n> nothing to commit, working tree clean\n> $ git branch --unset-upstream\n> fatal: could not unset 'branch.yves/xxx.merge'\n> $ git status\n> On branch yves/xxx\n> nothing to commit, working tree clean\n> $ git fetch\n> fatal: No remote repository specified. Please, specify either a URL or a\n> remote name from which new revisions should be fetched.\n\nAs a side note, I'm surprised that commands work at all when your\n.git/config is empty. I'd expect check_respository_format() to complain\nthat you are not in a repository. Looks like it is due to this block:\n\n 452\t\t /*\n 453\t\t * For historical use of check_repository_format() in git-init,\n 454\t\t * we treat a missing config as a silent \"ok\", even when nongit_ok\n 455\t\t * is unset.\n 456\t\t */\n 457\t\tif (candidate.version < 0)\n 458\t\t\treturn 0;\n\n> > No, it writes the new content to \"config.lock\" and then renames it into\n> > place.\n> > All of the write() calls to the temporary file are checked.\n> \n> I was going to say that perhaps the write was not checked... But if\n> you are confident they are then...\n\nYou're welcome to read over the function to double-check, but I just\nlooked it over and couldn't find any unchecked writes.\n\n> > Given that your output is consistent with it failing to find the key,\n> > and that the result is an empty file, it sounds like somehow the mmap'd\n> > input appeared empty (but neither open nor fstat nor mmap returned an\n> > error). You're not on any kind of exotic filesystem, are you?\n> \n> I don't think so, but I don't know. Is there a command I can run to check?\n> \n> BTW, with a bit of faffing I can probably recreate this problem.\n> Should I try? Is there something I could do during recreation that\n> would help?\n\nIf you think you can reproduce, the output of \"strace\" on a failing\ninvocation would be very interesting.\n\n-Peff\n"},{"id":"327939","messageId":"CANgJU+WHr2-FjAUUKQ8pUkniczcxrr11dTo_zBeXdTi_su2oUA@mail.gmail.com","threadId":"46742","inReplyTo":"20170913141739.mkmiaud2eemgnbbh@sigill.intra.peff.net","subject":"Re: Bug: git branch --unset-upstream command can nuke config when disk is full.","fromName":"demerphq","fromEmail":"demerphq@gmail.com","sentAt":"2017-09-13T14:49:45Z","receivedAt":"2017-09-13T14:49:53Z","isPatch":false,"sender":{"key":"demerphq@gmail.com","avatar":null},"body":"On 13 September 2017 at 16:17, Jeff King wrote:\n> You're welcome to read over the function to double-check, but I just\n> looked it over and couldn't find any unchecked writes.\n\nI can look, but I doubt I would notice something you did not.\n\nOn the other hand the strace output does show that this is a case\nwhere the writes failed, but we still renamed the empty config.lock\nfile into place:\n\n\nwrite(3, \"[core]\\n\\tsharedRepository = true\\n\"..., 288) = -1 ENOSPC\n(No space left on device)\nwrite(3, \" merge = refs/heads/yves/\"..., 51) = -1 ENOSPC (No\nspace left on device)\nmunmap(0x7f48d9b8c000, 363) = 0\nclose(3) = 0\nrename(\"/usr/local/git_tree/main/.git/config.lock\",\n\"/usr/local/git_tree/main/.git/config\") = 0\n\nFull strace is below:\n\n>> > Given that your output is consistent with it failing to find the key,\n>> > and that the result is an empty file, it sounds like somehow the mmap'd\n>> > input appeared empty (but neither open nor fstat nor mmap returned an\n>> > error). You're not on any kind of exotic filesystem, are you?\n>>\n>> I don't think so, but I don't know. Is there a command I can run to check?\n\nI freed up space and things worked, so I somehow doubt the filesystem\nis at fault. When I then filled up the disk and retried the error was\nrepeatable.\n\n>> BTW, with a bit of faffing I can probably recreate this problem.\n>> Should I try? Is there something I could do during recreation that\n>> would help?\n>\n> If you think you can reproduce, the output of \"strace\" on a failing\n> invocation would be very interesting.\n\nI can reproduce, see below. Preceded and suffixed by ls on the .git/config file.\n\nI have munged the branch name for privacy reasons, hope that doesn't\ninvalidate the strace utility.\n\ncheers,\nyves\n\n$ ls -la .git/config\n-rw-rw-r-- 1 root users 363 Sep 13 16:36 .git/config\n$ strace git branch --unset-upstream\nexecve(\"/usr/bin/git\", [\"git\", \"branch\", \"--unset-upstream\"], [/* 39\nvars */]) = 0\nbrk(0) = 0x222c000\nmmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,\n0) = 0x7f48d9b94000\naccess(\"/etc/ld.so.preload\", R_OK) = -1 ENOENT (No such file or directory)\nopen(\"/etc/ld.so.cache\", O_RDONLY) = 3\nfstat(3, {st_mode=S_IFREG|0644, st_size=50300, ...}) = 0\nmmap(NULL, 50300, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7f48d9b87000\nclose(3) = 0\nopen(\"/lib64/libpcre.so.0\", O_RDONLY) = 3\nread(3, \"\\177ELF\\2\\1\\1\\0\\0\\0\\0\\0\\0\\0\\0\\0\\3\\0>\\0\\1\\0\\0\\0@\\25\\0\\0\\0\\0\\0\\0\"...,\n832) = 832\nfstat(3, {st_mode=S_IFREG|0755, st_size=183080, ...}) = 0\nmmap(NULL, 2278264, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3,\n0) = 0x7f48d9748000\nmprotect(0x7f48d9774000, 2097152, PROT_NONE) = 0\nmmap(0x7f48d9974000, 4096, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x2c000) = 0x7f48d9974000\nclose(3) = 0\nopen(\"/lib64/libz.so.1\", O_RDONLY) = 3\nread(3, \"\\177ELF\\2\\1\\1\\0\\0\\0\\0\\0\\0\\0\\0\\0\\3\\0>\\0\\1\\0\\0\\0\n!\\0\\0\\0\\0\\0\\0\"..., 832) = 832\nfstat(3, {st_mode=S_IFREG|0755, st_size=88600, ...}) = 0\nmmap(NULL, 2183696, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3,\n0) = 0x7f48d9532000\nmprotect(0x7f48d9547000, 2093056, PROT_NONE) = 0\nmmap(0x7f48d9746000, 8192, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x14000) = 0x7f48d9746000\nclose(3) = 0\nopen(\"/lib64/libpthread.so.0\", O_RDONLY) = 3\nread(3, \"\\177ELF\\2\\1\\1\\0\\0\\0\\0\\0\\0\\0\\0\\0\\3\\0>\\0\\1\\0\\0\\0000^\\0\\0\\0\\0\\0\\0\"...,\n832) = 832\nfstat(3, {st_mode=S_IFREG|0755, st_size=143280, ...}) = 0\nmmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,\n0) = 0x7f48d9b86000\nmmap(NULL, 2212848, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3,\n0) = 0x7f48d9315000\nmprotect(0x7f48d932c000, 2097152, PROT_NONE) = 0\nmmap(0x7f48d952c000, 8192, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x17000) = 0x7f48d952c000\nmmap(0x7f48d952e000, 13296, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7f48d952e000\nclose(3) = 0\nopen(\"/lib64/librt.so.1\", O_RDONLY) = 3\nread(3, \"\\177ELF\\2\\1\\1\\0\\0\\0\\0\\0\\0\\0\\0\\0\\3\\0>\\0\\1\\0\\0\\0\\240!\\0\\0\\0\\0\\0\\0\"...,\n832) = 832\nfstat(3, {st_mode=S_IFREG|0755, st_size=44472, ...}) = 0\nmmap(NULL, 2128816, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3,\n0) = 0x7f48d910d000\nmprotect(0x7f48d9114000, 2093056, PROT_NONE) = 0\nmmap(0x7f48d9313000, 8192, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x6000) = 0x7f48d9313000\nclose(3) = 0\nopen(\"/lib64/libc.so.6\", O_RDONLY) = 3\nread(3, \"\\177ELF\\2\\1\\1\\3\\0\\0\\0\\0\\0\\0\\0\\0\\3\\0>\\0\\1\\0\\0\\0000\\356\\1\\0\\0\\0\\0\\0\"...,\n832) = 832\nfstat(3, {st_mode=S_IFREG|0755, st_size=1924768, ...}) = 0\nmmap(NULL, 3750184, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3,\n0) = 0x7f48d8d79000\nmprotect(0x7f48d8f03000, 2097152, PROT_NONE) = 0\nmmap(0x7f48d9103000, 24576, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x18a000) = 0x7f48d9103000\nmmap(0x7f48d9109000, 14632, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7f48d9109000\nclose(3) = 0\nmmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,\n0) = 0x7f48d9b85000\nmmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,\n0) = 0x7f48d9b84000\nmmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,\n0) = 0x7f48d9b83000\narch_prctl(ARCH_SET_FS, 0x7f48d9b84700) = 0\nmprotect(0x7f48d9103000, 16384, PROT_READ) = 0\nmprotect(0x7f48d9313000, 4096, PROT_READ) = 0\nmprotect(0x7f48d952c000, 4096, PROT_READ) = 0\nmprotect(0x7f48d9746000, 4096, PROT_READ) = 0\nmprotect(0x7f48d9b95000, 4096, PROT_READ) = 0\nmunmap(0x7f48d9b87000, 50300) = 0\nset_tid_address(0x7f48d9b849d0) = 50373\nset_robust_list(0x7f48d9b849e0, 24) = 0\nfutex(0x7fff059e43bc, FUTEX_WAKE_PRIVATE, 1) = 0\nfutex(0x7fff059e43bc, FUTEX_WAIT_BITSET_PRIVATE|FUTEX_CLOCK_REALTIME,\n1, NULL, 7f48d9b84700) = -1 EAGAIN (Resource temporarily unavailable)\nrt_sigaction(SIGRTMIN, {0x7f48d931acb0, [], SA_RESTORER|SA_SIGINFO,\n0x7f48d93247e0}, NULL, 8) = 0\nrt_sigaction(SIGRT_1, {0x7f48d931ad40, [],\nSA_RESTORER|SA_RESTART|SA_SIGINFO, 0x7f48d93247e0}, NULL, 8) = 0\nrt_sigprocmask(SIG_UNBLOCK, [RTMIN RT_1], NULL, 8) = 0\ngetrlimit(RLIMIT_STACK, {rlim_cur=10240*1024, rlim_max=RLIM64_INFINITY}) = 0\nopen(\"/dev/null\", O_RDWR) = 3\nclose(3) = 0\nbrk(0) = 0x222c000\nbrk(0x224d000) = 0x224d000\nopen(\"/usr/lib/locale/locale-archive\", O_RDONLY) = 3\nfstat(3, {st_mode=S_IFREG|0644, st_size=99170352, ...}) = 0\nmmap(NULL, 99170352, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7f48d2ee5000\nclose(3) = 0\nopen(\"/usr/lib64/gconv/gconv-modules.cache\", O_RDONLY) = 3\nfstat(3, {st_mode=S_IFREG|0644, st_size=26060, ...}) = 0\nmmap(NULL, 26060, PROT_READ, MAP_SHARED, 3, 0) = 0x7f48d9b8d000\nclose(3) = 0\nfutex(0x7f48d9108828, FUTEX_WAKE_PRIVATE, 2147483647) = 0\nrt_sigprocmask(SIG_UNBLOCK, [PIPE], NULL, 8) = 0\nrt_sigaction(SIGPIPE, {SIG_DFL, [PIPE], SA_RESTORER|SA_RESTART,\n0x7f48d8dab510}, {SIG_DFL, [], 0}, 8) = 0\ngetcwd(\"/usr/local/git_tree/main\", 129) = 25\nstat(\"/usr/local/git_tree/main\", {st_mode=S_IFDIR|S_ISGID|0775,\nst_size=4096, ...}) = 0\nstat(\"/usr/local/git_tree/main/.git\", {st_mode=S_IFDIR|S_ISGID|0775,\nst_size=4096, ...}) = 0\nlstat(\"/usr/local/git_tree/main/.git/HEAD\", {st_mode=S_IFREG|0664,\nst_size=39, ...}) = 0\nopen(\"/usr/local/git_tree/main/.git/HEAD\", O_RDONLY) = 3\nread(3, \"ref: refs/heads/yves/simple_proj\"..., 255) = 39\nread(3, \"\", 216) = 0\nclose(3) = 0\nlstat(\"/usr/local/git_tree/main/.git/commondir\", 0x7fff059e3ec0) = -1\nENOENT (No such file or directory)\naccess(\"/usr/local/git_tree/main/.git/objects\", X_OK) = 0\naccess(\"/usr/local/git_tree/main/.git/refs\", X_OK) = 0\nlstat(\".git/commondir\", 0x7fff059e3ef0) = -1 ENOENT (No such file or directory)\nopen(\".git/config\", O_RDONLY) = 3\nfstat(3, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0\nfstat(3, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0\nmmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,\n0) = 0x7f48d9b8c000\nread(3, \"[core]\\n\\tsharedRepository = true\\n\"..., 4096) = 363\nread(3, \"\", 4096) = 0\nclose(3) = 0\nmunmap(0x7f48d9b8c000, 4096) = 0\ngetcwd(\"/usr/local/git_tree/main\", 129) = 25\nstat(\".git\", {st_mode=S_IFDIR|S_ISGID|0775, st_size=4096, ...}) = 0\nlstat(\".git/commondir\", 0x7fff059e3f50) = -1 ENOENT (No such file or directory)\naccess(\"/etc/gitconfig\", R_OK) = 0\nopen(\"/etc/gitconfig\", O_RDONLY) = 3\nfstat(3, {st_mode=S_IFREG|0644, st_size=1353, ...}) = 0\nfstat(3, {st_mode=S_IFREG|0644, st_size=1353, ...}) = 0\nmmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,\n0) = 0x7f48d9b8c000\nread(3, \"################################\"..., 4096) = 1353\nread(3, \"\", 4096) = 0\nclose(3) = 0\nmunmap(0x7f48d9b8c000, 4096) = 0\naccess(\"/home/yorton/.config/git/config\", R_OK) = -1 ENOENT (No such\nfile or directory)\naccess(\"/home/yorton/.gitconfig\", R_OK) = 0\nopen(\"/home/yorton/.gitconfig\", O_RDONLY) = 3\nfstat(3, {st_mode=S_IFREG|0644, st_size=377, ...}) = 0\nfstat(3, {st_mode=S_IFREG|0644, st_size=377, ...}) = 0\nmmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,\n0) = 0x7f48d9b8c000\nread(3, \"[alias]\\n\\thist = log --graph --co\"..., 4096) = 377\nread(3, \"\", 4096) = 0\nclose(3) = 0\nmunmap(0x7f48d9b8c000, 4096) = 0\naccess(\".git/config\", R_OK) = 0\nopen(\".git/config\", O_RDONLY) = 3\nfstat(3, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0\nfstat(3, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0\nmmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,\n0) = 0x7f48d9b8c000\nread(3, \"[core]\\n\\tsharedRepository = true\\n\"..., 4096) = 363\nread(3, \"\", 4096) = 0\nclose(3) = 0\nmunmap(0x7f48d9b8c000, 4096) = 0\nopen(\"/usr/share/locale/locale.alias\", O_RDONLY|O_CLOEXEC) = 3\nfstat(3, {st_mode=S_IFREG|0644, st_size=2512, ...}) = 0\nmmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,\n0) = 0x7f48d9b8c000\nread(3, \"# Locale name alias data base.\\n#\"..., 4096) = 2512\nread(3, \"\", 4096) = 0\nclose(3) = 0\nmunmap(0x7f48d9b8c000, 4096) = 0\nopen(\"/usr/share/locale/en_CA.utf8/LC_MESSAGES/git.mo\", O_RDONLY) = -1\nENOENT (No such file or directory)\nopen(\"/usr/share/locale/en_CA/LC_MESSAGES/git.mo\", O_RDONLY) = -1\nENOENT (No such file or directory)\nopen(\"/usr/share/locale/en.utf8/LC_MESSAGES/git.mo\", O_RDONLY) = -1\nENOENT (No such file or directory)\nopen(\"/usr/share/locale/en/LC_MESSAGES/git.mo\", O_RDONLY) = -1 ENOENT\n(No such file or directory)\naccess(\"/etc/gitconfig\", R_OK) = 0\nopen(\"/etc/gitconfig\", O_RDONLY) = 3\nfstat(3, {st_mode=S_IFREG|0644, st_size=1353, ...}) = 0\nfstat(3, {st_mode=S_IFREG|0644, st_size=1353, ...}) = 0\nmmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,\n0) = 0x7f48d9b8c000\nread(3, \"################################\"..., 4096) = 1353\nread(3, \"\", 4096) = 0\nclose(3) = 0\nmunmap(0x7f48d9b8c000, 4096) = 0\naccess(\"/home/yorton/.config/git/config\", R_OK) = -1 ENOENT (No such\nfile or directory)\naccess(\"/home/yorton/.gitconfig\", R_OK) = 0\nopen(\"/home/yorton/.gitconfig\", O_RDONLY) = 3\nfstat(3, {st_mode=S_IFREG|0644, st_size=377, ...}) = 0\nfstat(3, {st_mode=S_IFREG|0644, st_size=377, ...}) = 0\nmmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,\n0) = 0x7f48d9b8c000\nread(3, \"[alias]\\n\\thist = log --graph --co\"..., 4096) = 377\nread(3, \"\", 4096) = 0\nclose(3) = 0\nmunmap(0x7f48d9b8c000, 4096) = 0\naccess(\".git/config\", R_OK) = 0\nopen(\".git/config\", O_RDONLY) = 3\nfstat(3, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0\nfstat(3, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0\nmmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,\n0) = 0x7f48d9b8c000\nread(3, \"[core]\\n\\tsharedRepository = true\\n\"..., 4096) = 363\nread(3, \"\", 4096) = 0\nclose(3) = 0\nmunmap(0x7f48d9b8c000, 4096) = 0\nlstat(\".git/commondir\", 0x7fff059e3590) = -1 ENOENT (No such file or directory)\nlstat(\".git/HEAD\", {st_mode=S_IFREG|0664, st_size=39, ...}) = 0\nopen(\".git/HEAD\", O_RDONLY) = 3\nread(3, \"ref: refs/heads/yves/xxx\"..., 256) = 39\nread(3, \"\", 217) = 0\nclose(3) = 0\nlstat(\".git/refs/heads/yves/xxx\", 0x7fff059e3580) = -1 ENOENT (No such\nfile or directory)\nopen(\".git/packed-refs\", O_RDONLY) = 3\nfstat(3, {st_mode=S_IFREG|0664, st_size=231354, ...}) = 0\nfstat(3, {st_mode=S_IFREG|0664, st_size=231354, ...}) = 0\nfstat(3, {st_mode=S_IFREG|0664, st_size=231354, ...}) = 0\nmmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,\n0) = 0x7f48d9b8c000\nread(3, \"# pack-refs with: peeled fully-p\"..., 4096) = 4096\nread(3, \"1a5c122f68f04fae374ac3cdf11\\n14e0\"..., 4096) = 4096\nread(3, \"b16f20dde39f97ef2869296\\nba29d061\"..., 4096) = 4096\nread(3, \"6-141414\\n^29ba731b77bb5f5c15644d\"..., 4096) = 4096\nread(3, \"df2d5e\\n3cd1ca6cffc2ce48ecc09c0c1\"..., 4096) = 4096\nread(3, \"fd414d360b4d742e8711fe8093a09fa \"..., 4096) = 4096\nread(3, \"pit-20170511-164552\\n^cb28295c135\"..., 4096) = 4096\nread(3, \"4a4cb2cf8620771feb\\n41b2f890ea48b\"..., 4096) = 4096\nread(3, \"\\n^9827bb20ed16ec66dd510d72d779c6\"..., 4096) = 4096\nread(3, \"-160507\\n^56472ff6a42c58dee27d337\"..., 4096) = 4096\nread(3, \"0d01a69f3c0\\nef9052d4b9c0e143a5f0\"..., 4096) = 4096\nread(3, \"/tags/dummyapp-20170609-182243_a\"..., 4096) = 4096\nread(3, \"5418485a012396832faedc5\\nc697717c\"..., 4096) = 4096\nread(3, \"3-141556\\n^69219f5ddd74c43d5574e5\"..., 4096) = 4096\nread(3, \"0602-122026\\n^4cbb642a2b3cb305336\"..., 4096) = 4096\nread(3, \"a9bee28a76502716da588fd2fc22020a\"..., 4096) = 4096\nread(3, \"73a677f refs/tags/events-2016090\"..., 4096) = 4096\nread(3, \"3be9fd87d5bf6217aaf195d5\\n650866a\"..., 4096) = 4096\nread(3, \"a95cda22bc90a79\\n4f3a36e9adac266f\"..., 4096) = 4096\nread(3, \"ec1097435aeed7ed8\\n7a2c97e8ad1a9f\"..., 4096) = 4096\nread(3, \"dba7614549cec3ed80803b6b01161c3d\"..., 4096) = 4096\nread(3, \"4ef8e8f\\n7e3c15cd86655da442c48ba7\"..., 4096) = 4096\nread(3, \"1d057a4b7dc47ac09f40a8d31916218\\n\"..., 4096) = 4096\nread(3, \"s/tags/intercomcron-20170615-110\"..., 4096) = 4096\nread(3, \"\\n^43f080925840cf77ae016fa93ecbdf\"..., 4096) = 4096\nread(3, \"gs/logproc-20161230-121402\\n^a25f\"..., 4096) = 4096\nread(3, \"2d55c54c708fca2cfe726e72d86ad50 \"..., 4096) = 4096\nread(3, \"70612-163608\\n^614fcefc7f4c39b12f\"..., 4096) = 4096\nread(3, \"ilebuild-20170512-145808\\n^47a87c\"..., 4096) = 4096\nbrk(0x226e000) = 0x226e000\nread(3, \"7a428de652f5a3c9c4\\n214d8d1128260\"..., 4096) = 4096\nread(3, \"\\n^e5486659e4bad48a3fb1177b619f11\"..., 4096) = 4096\nread(3, \"91f33991c3efed545f51 refs/tags/p\"..., 4096) = 4096\nread(3, \"08d313fb454\\nefe6a551adac01f51538\"..., 4096) = 4096\nread(3, \"/ppcapp-20170628-111257\\n^f56019d\"..., 4096) = 4096\nread(3, \"b50901 refs/tags/provdash-201706\"..., 4096) = 4096\nread(3, \"7 refs/tags/rtmapp-20170613-1102\"..., 4096) = 4096\nread(3, \"-192153\\n^296cc5ffc1c84b049fd0a15\"..., 4096) = 4096\nread(3, \"a6\\nda231495f5708ecdafe41aedf28a4\"..., 4096) = 4096\nread(3, \"202\\n^70ebec46ba2859c12b67a6dca82\"..., 4096) = 4096\nread(3, \"9d54a38eeff507e45d\\n9d93a8eaf70d3\"..., 4096) = 4096\nread(3, \"ecure-hmsapp-20161021-181248\\n^a5\"..., 4096) = 4096\nread(3, \"0111-134153\\n^13d748af46df47b5315\"..., 4096) = 4096\nread(3, \"1 refs/tags/soylent-20170606-154\"..., 4096) = 4096\nread(3, \"tags/statprofileapp-20161104-125\"..., 4096) = 4096\nread(3, \"tags/substreamproc-20170214-1429\"..., 4096) = 4096\nread(3, \"efs/tags/suite_partner_sites-201\"..., 4096) = 4096\nread(3, \"167 refs/tags/tokenauth-20160929\"..., 4096) = 4096\nread(3, \"640\\n^00b52cdaaaf3c55e069cb62740d\"..., 4096) = 4096\nread(3, \"9753e496ebbf7b05f009dd32\\ndb917ed\"..., 4096) = 4096\nread(3, \"8cc96a8a9313d82cc4c91d2ddc31613b\"..., 4096) = 4096\nread(3, \"2ad87b80a7a0\\n56519d1a3c4fe5cdcc3\"..., 4096) = 4096\nread(3, \" refs/tags/xml-secure-mobile-201\"..., 4096) = 4096\nread(3, \"794a5b25d4b5d411cc736298a\\nb4cb1c\"..., 4096) = 4096\nread(3, \"057d938c27f8b16e6ef9839874f696a2\"..., 4096) = 4096\nread(3, \"3f3259ffc0262e4bf56f4eec01c76ac8\"..., 4096) = 4096\nread(3, \"\\n^dbab804616296abf788d4a1d991b3b\"..., 4096) = 4096\nread(3, \"dad60d1e168e7d7de94e3da1 refs/ta\"..., 4096) = 1978\nread(3, \"\", 4096) = 0\nclose(3) = 0\nmunmap(0x7f48d9b8c000, 4096) = 0\nlstat(\".git/HEAD\", {st_mode=S_IFREG|0664, st_size=39, ...}) = 0\nopen(\".git/HEAD\", O_RDONLY) = 3\nread(3, \"ref: refs/heads/yves/xxx\"..., 256) = 39\nread(3, \"\", 217) = 0\nclose(3) = 0\nlstat(\".git/refs/heads/yves/xxx\", 0x7fff059e34b0) = -1 ENOENT (No such\nfile or directory)\nstat(\".git/packed-refs\", {st_mode=S_IFREG|0664, st_size=231354, ...}) = 0\nreadlink(\".git/config\", 0x2267b40, 32) = -1 EINVAL (Invalid argument)\nrt_sigaction(SIGINT, {0x54a550, [INT], SA_RESTORER|SA_RESTART,\n0x7f48d8dab510}, {SIG_DFL, [], 0}, 8) = 0\nrt_sigaction(SIGHUP, {0x54a550, [HUP], SA_RESTORER|SA_RESTART,\n0x7f48d8dab510}, {SIG_DFL, [], 0}, 8) = 0\nrt_sigaction(SIGTERM, {0x54a550, [TERM], SA_RESTORER|SA_RESTART,\n0x7f48d8dab510}, {SIG_DFL, [], 0}, 8) = 0\nrt_sigaction(SIGQUIT, {0x54a550, [QUIT], SA_RESTORER|SA_RESTART,\n0x7f48d8dab510}, {SIG_DFL, [], 0}, 8) = 0\nrt_sigaction(SIGPIPE, {0x54a550, [PIPE], SA_RESTORER|SA_RESTART,\n0x7f48d8dab510}, {SIG_DFL, [PIPE], SA_RESTORER|SA_RESTART,\n0x7f48d8dab510}, 8) = 0\ngetcwd(\"/usr/local/git_tree/main\", 129) = 25\nopen(\"/usr/local/git_tree/main/.git/config.lock\",\nO_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3\nlstat(\"/usr/local/git_tree/main/.git/config.lock\",\n{st_mode=S_IFREG|0664, st_size=0, ...}) = 0\nopen(\".git/config\", O_RDONLY) = 4\nopen(\".git/config\", O_RDONLY) = 5\nfstat(5, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0\nfstat(5, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0\nmmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,\n0) = 0x7f48d9b8c000\nread(5, \"[core]\\n\\tsharedRepository = true\\n\"..., 4096) = 363\nlseek(5, 0, SEEK_CUR) = 363\nread(5, \"\", 4096) = 0\nclose(5) = 0\nmunmap(0x7f48d9b8c000, 4096) = 0\nfstat(4, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0\nmmap(NULL, 363, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7f48d9b8c000\nclose(4) = 0\nchmod(\"/usr/local/git_tree/main/.git/config.lock\", 0664) = 0\nwrite(3, \"[core]\\n\\tsharedRepository = true\\n\"..., 288) = -1 ENOSPC\n(No space left on device)\nwrite(3, \" merge = refs/heads/yves/\"..., 51) = -1 ENOSPC (No\nspace left on device)\nmunmap(0x7f48d9b8c000, 363) = 0\nclose(3) = 0\nrename(\"/usr/local/git_tree/main/.git/config.lock\",\n\"/usr/local/git_tree/main/.git/config\") = 0\nreadlink(\".git/config\", 0x2230d00, 32) = -1 EINVAL (Invalid argument)\ngetcwd(\"/usr/local/git_tree/main\", 129) = 25\nopen(\"/usr/local/git_tree/main/.git/config.lock\",\nO_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3\nlstat(\"/usr/local/git_tree/main/.git/config.lock\",\n{st_mode=S_IFREG|0664, st_size=0, ...}) = 0\nopen(\".git/config\", O_RDONLY) = 4\nopen(\".git/config\", O_RDONLY) = 5\nfstat(5, {st_mode=S_IFREG|0664, st_size=0, ...}) = 0\nfstat(5, {st_mode=S_IFREG|0664, st_size=0, ...}) = 0\nmmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,\n0) = 0x7f48d9b8c000\nread(5, \"\", 4096) = 0\nclose(5) = 0\nmunmap(0x7f48d9b8c000, 4096) = 0\nclose(3) = 0\nunlink(\"/usr/local/git_tree/main/.git/config.lock\") = 0\nclose(4) = 0\nwrite(2, \"fatal: could not unset 'branch.y\"..., 61fatal: could not\nunset 'branch.yves/xxx.merge'\n) = 61\nexit_group(128) = ?\n+++ exited with 128 +++\n$ ls -la .git/config\n-rw-rw-r-- 1 yorton users 0 Sep 13 16:38 .git/config\n\n\n\n-- \nperl -Mre=debug -e \"/just|another|perl|hacker/\"\n"},{"id":"327940","messageId":"20170913145154.423zqe6antzccclk@sigill.intra.peff.net","threadId":"46742","inReplyTo":"CANgJU+WHr2-FjAUUKQ8pUkniczcxrr11dTo_zBeXdTi_su2oUA@mail.gmail.com","subject":"Re: Bug: git branch --unset-upstream command can nuke config when disk is full.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T14:51:54Z","receivedAt":"2017-09-13T14:52:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2017 at 04:49:45PM +0200, demerphq wrote:\n\n> On 13 September 2017 at 16:17, Jeff King wrote:\n> > You're welcome to read over the function to double-check, but I just\n> > looked it over and couldn't find any unchecked writes.\n> \n> I can look, but I doubt I would notice something you did not.\n> \n> On the other hand the strace output does show that this is a case\n> where the writes failed, but we still renamed the empty config.lock\n> file into place:\n> \n> \n> write(3, \"[core]\\n\\tsharedRepository = true\\n\"..., 288) = -1 ENOSPC\n> (No space left on device)\n> write(3, \" merge = refs/heads/yves/\"..., 51) = -1 ENOSPC (No\n> space left on device)\n> munmap(0x7f48d9b8c000, 363) = 0\n> close(3) = 0\n> rename(\"/usr/local/git_tree/main/.git/config.lock\",\n> \"/usr/local/git_tree/main/.git/config\") = 0\n\nHmph. That is very disturbing. But with that information I should be\nable to track down the culprit. Thanks for digging.\n\n> I freed up space and things worked, so I somehow doubt the filesystem\n> is at fault. When I then filled up the disk and retried the error was\n> repeatable.\n\nYeah, agreed. This really does look like a bug.\n\n-Peff\n"},{"id":"327943","messageId":"CANgJU+XvJA2LYgFgmJZw7BR4-7qobvOh5zOSAVzGFOTsJ=Z8ug@mail.gmail.com","threadId":"46742","inReplyTo":"20170913145154.423zqe6antzccclk@sigill.intra.peff.net","subject":"Re: Bug: git branch --unset-upstream command can nuke config when disk is full.","fromName":"demerphq","fromEmail":"demerphq@gmail.com","sentAt":"2017-09-13T15:18:56Z","receivedAt":"2017-09-13T15:19:01Z","isPatch":false,"sender":{"key":"demerphq@gmail.com","avatar":null},"body":"On 13 September 2017 at 16:51, Jeff King wrote:\n> On Wed, Sep 13, 2017 at 04:49:45PM +0200, demerphq wrote:\n>\n>> On 13 September 2017 at 16:17, Jeff King wrote:\n>> > You're welcome to read over the function to double-check, but I just\n>> > looked it over and couldn't find any unchecked writes.\n>>\n>> I can look, but I doubt I would notice something you did not.\n>>\n>> On the other hand the strace output does show that this is a case\n>> where the writes failed, but we still renamed the empty config.lock\n>> file into place:\n>>\n>>\n>> write(3, \"[core]\\n\\tsharedRepository = true\\n\"..., 288) = -1 ENOSPC\n>> (No space left on device)\n>> write(3, \" merge = refs/heads/yves/\"..., 51) = -1 ENOSPC (No\n>> space left on device)\n>> munmap(0x7f48d9b8c000, 363) = 0\n>> close(3) = 0\n>> rename(\"/usr/local/git_tree/main/.git/config.lock\",\n>> \"/usr/local/git_tree/main/.git/config\") = 0\n>\n> Hmph. That is very disturbing. But with that information I should be\n> able to track down the culprit. Thanks for digging.\n\nFWIW, I see that git_config_set_multivar_in_file_gently() uses\nwrite_in_full() which in turn uses xwrite(), but the latter has the\nfollowing comment on it:\n\n/*\n * xwrite() is the same a write(), but it automatically restarts write()\n * operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT\n * GUARANTEE that \"len\" bytes is written even if the operation is successful.\n */\n\nI suspect that at this point I am not adding much value here, so I\nwill leave it at this.\n\n>> I freed up space and things worked, so I somehow doubt the filesystem\n>> is at fault. When I then filled up the disk and retried the error was\n>> repeatable.\n>\n> Yeah, agreed. This really does look like a bug.\n\nFWIW, where it bit me turned out to be harmless. So while no doubt\nthis could be a real PITA for someone it wasn't for me.\n\nYves\n\n\n-- \nperl -Mre=debug -e \"/just|another|perl|hacker/\"\n"},{"id":"327944","messageId":"20170913152205.sw3i5xzsynx62nea@sigill.intra.peff.net","threadId":"46742","inReplyTo":"CANgJU+XvJA2LYgFgmJZw7BR4-7qobvOh5zOSAVzGFOTsJ=Z8ug@mail.gmail.com","subject":"Re: Bug: git branch --unset-upstream command can nuke config when disk is full.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T15:22:05Z","receivedAt":"2017-09-13T15:22:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2017 at 05:18:56PM +0200, demerphq wrote:\n\n> > Hmph. That is very disturbing. But with that information I should be\n> > able to track down the culprit. Thanks for digging.\n> \n> FWIW, I see that git_config_set_multivar_in_file_gently() uses\n> write_in_full() which in turn uses xwrite(), but the latter has the\n> following comment on it:\n> \n> /*\n> * xwrite() is the same a write(), but it automatically restarts write()\n> * operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT\n> * GUARANTEE that \"len\" bytes is written even if the operation is successful.\n> */\n> \n> I suspect that at this point I am not adding much value here, so I\n> will leave it at this.\n\nNo, the problem is in this line:\n\n if (write_in_full(fd, contents + copy_begin,\n copy_end - copy_begin) <\n copy_end - copy_begin)\n goto write_err_out;\n\nwrite_in_full() returns -1 on error (_not_ how many bytes were actually\nwritten). So its return is a signed ssize_t. But the result of the\npointer subtraction \"copy_end - copy_begin\" is an unsigned ptrdiff_t.\nThe compiler promotes the signed to an unsigned, so the condition can\nnever be true (the \"-1\" becomes the highest possible value).\n\nI have the fix, but I'm searching the code base for other instances of\nthe same error.\n\n-Peff\n"},{"id":"327945","messageId":"CANgJU+XyfeDCpdZTKJOppjs6w6zS1kjxDAwbvOZr_1a8Fyqe7Q@mail.gmail.com","threadId":"46742","inReplyTo":"20170913152205.sw3i5xzsynx62nea@sigill.intra.peff.net","subject":"Re: Bug: git branch --unset-upstream command can nuke config when disk is full.","fromName":"demerphq","fromEmail":"demerphq@gmail.com","sentAt":"2017-09-13T15:49:14Z","receivedAt":"2017-09-13T15:49:19Z","isPatch":false,"sender":{"key":"demerphq@gmail.com","avatar":null},"body":"On 13 September 2017 at 17:22, Jeff King wrote:\n> On Wed, Sep 13, 2017 at 05:18:56PM +0200, demerphq wrote:\n>\n>> > Hmph. That is very disturbing. But with that information I should be\n>> > able to track down the culprit. Thanks for digging.\n>>\n>> FWIW, I see that git_config_set_multivar_in_file_gently() uses\n>> write_in_full() which in turn uses xwrite(), but the latter has the\n>> following comment on it:\n>>\n>> /*\n>> * xwrite() is the same a write(), but it automatically restarts write()\n>> * operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT\n>> * GUARANTEE that \"len\" bytes is written even if the operation is successful.\n>> */\n>>\n>> I suspect that at this point I am not adding much value here, so I\n>> will leave it at this.\n>\n> No, the problem is in this line:\n>\n> if (write_in_full(fd, contents + copy_begin,\n> copy_end - copy_begin) <\n> copy_end - copy_begin)\n> goto write_err_out;\n>\n> write_in_full() returns -1 on error (_not_ how many bytes were actually\n> written). So its return is a signed ssize_t. But the result of the\n> pointer subtraction \"copy_end - copy_begin\" is an unsigned ptrdiff_t.\n> The compiler promotes the signed to an unsigned, so the condition can\n> never be true (the \"-1\" becomes the highest possible value).\n\nBah. Good eye. I missed that entirely.\n\n> I have the fix, but I'm searching the code base for other instances of\n> the same error.\n\nYeah, I think there are few just in that file.\n\nThanks for fixing this!\n\ncheers,\nYves\n\n-- \nperl -Mre=debug -e \"/just|another|perl|hacker/\"\n"},{"id":"327947","messageId":"20170913170807.cyx7rrpoyhaauvol@sigill.intra.peff.net","threadId":"46742","inReplyTo":"CANgJU+Wj_sN8Px6rweHMw_iAX8UA3mW9x4NVCctpUCs8_3ytLw@mail.gmail.com","subject":"[PATCH 0/7] config.c may fail to notice some write() failures","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T17:08:07Z","receivedAt":"2017-09-13T17:08:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2017 at 01:59:17PM +0200, demerphq wrote:\n\n> After being away for a while I saw the following message in one of my git repos:\n> \n> $ git status\n> On branch yves/xxx\n> Your branch is based on 'origin/yves/xxx', but the upstream is gone.\n> (use \"git branch --unset-upstream\" to fixup)\n> \n> nothing to commit, working tree clean\n> $ git branch --unset-upstream\n> fatal: could not unset 'branch.yves/simple_projection.merge'\n> \n> At this point my .git/config file was empty, and all of my config was lost.\n\nHere's the fix, along with some related cleanups. The actual bugfix is\nin the first patch, which I think should go to \"maint\". The rest are not\nas important, so could go to master (but would also be fine for maint,\nas there should be no user-visible changes).\n\n [1/7]: config: avoid \"write_in_full(fd, buf, len) < len\" pattern\n [2/7]: get-tar-commit-id: check write_in_full() return against 0\n [3/7]: avoid \"write_in_full(fd, buf, len) != len\" pattern\n [4/7]: convert less-trivial versions of \"write_in_full() != len\"\n [5/7]: pkt-line: check write_in_full() errors against \"< 0\"\n [6/7]: notes-merge: use ssize_t for write_in_full() return value\n [7/7]: config: flip return value of store_write_*()\n\n builtin/get-tar-commit-id.c | 3 +--\n builtin/receive-pack.c | 2 +-\n builtin/rerere.c | 2 +-\n builtin/unpack-file.c | 2 +-\n config.c | 38 +++++++++++++++++++-------------------\n diff.c | 2 +-\n entry.c | 5 +++--\n fast-import.c | 2 +-\n http-backend.c | 4 ++--\n ll-merge.c | 2 +-\n notes-merge.c | 2 +-\n pkt-line.c | 29 ++++++++++++++---------------\n read-cache.c | 6 +++---\n refs.c | 2 +-\n refs/files-backend.c | 10 +++++-----\n rerere.c | 2 +-\n shallow.c | 6 +++---\n streaming.c | 2 +-\n t/helper/test-delta.c | 2 +-\n transport-helper.c | 5 ++---\n wrapper.c | 2 +-\n 21 files changed, 64 insertions(+), 66 deletions(-)\n\n-Peff\n"},{"id":"327949","messageId":"20170913171104.yu7ags4aq2zdwz6r@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913170807.cyx7rrpoyhaauvol@sigill.intra.peff.net","subject":"[PATCH 1/7] config: avoid \"write_in_full(fd, buf, len) < len\" pattern","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T17:11:05Z","receivedAt":"2017-09-13T17:11:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The return type of write_in_full() is a signed ssize_t,\nbecause we may return \"-1\" on failure (even if we succeeded\nin writing some bytes). But \"len\" itself may be an\nunsigned type (the function takes a size_t, but of course we\nmay have something else in the calling function). So while\nit seems like:\n\n if (write_in_full(fd, buf, len) < len)\n\tdie_errno(\"write error\");\n\nwould notice an error, it won't if \"len\" is unsigned. The\ncompiler sees a signed/unsigned comparison and promotes the\nsigned value, resulting in (size_t)-1, the highest possible\nsize_t (or again, whatever type the caller has). This cannot\npossibly be smaller than \"len\", and so the conditional can\nnever trigger.\n\nI scoured the code base for cases of this, but it turns out\nthat these two in git_config_set_multivar_in_file_gently()\nare the only ones. This case is actually quite interesting:\nwe don't have a size_t, but rather use the subtraction of\ntwo pointers. Which you might think would be a signed\nptrdiff_t, but clearly both gcc and clang treat it as\nunsigned (possibly because the conditional just above\nguarantees that the result is greater than zero).\n\nWe can avoid the whole question by just checking for a\nnegative return value directly, as write_in_full() will\nnever return any value except -1 or the full count.\n\nThere's no addition to the test suite here, since you need\nto convince write() to fail in order to see the problem. The\nsimplest reproduction recipe I came up with is to trigger\nENOSPC (this only works on Linux, obviously):\n\n # make a limited-size filesystem\n dd if=/dev/zero of=small.disk bs=1M count=1\n mke2fs small.disk\n mkdir mnt\n sudo mount -o loop small.disk mnt\n cd mnt\n sudo chown $USER:$USER .\n\n # make a config file with some content\n git config --file=config one.key value\n git config --file=config two.key value\n\n # now fill up the disk\n dd if=/dev/zero of=fill\n\n # and try to delete a key, which requires copying the rest\n # of the file to config.lock, and will fail on write()\n git config --file=config --unset two.key\n\nThat final command should (and does after this patch)\nproduce an error message due to the failed write, and leave\nthe file intact. With the current code it silently ignores\nthe failure and renames config.lock into place, leaving you\nwith a totally empty config file!\n\nReported-by: demerphq \nSigned-off-by: Jeff King \n---\n config.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex d0d8ce823a..eee4ac0355 100644\n--- a/config.c\n+++ b/config.c\n@@ -2608,8 +2608,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\t/* write the first part of the config */\n \t\t\tif (copy_end > copy_begin) {\n \t\t\t\tif (write_in_full(fd, contents + copy_begin,\n-\t\t\t\t\t\t copy_end - copy_begin) <\n-\t\t\t\t copy_end - copy_begin)\n+\t\t\t\t\t\t copy_end - copy_begin) < 0)\n \t\t\t\t\tgoto write_err_out;\n \t\t\t\tif (new_line &&\n \t\t\t\t write_str_in_full(fd, \"\\n\") != 1)\n@@ -2631,8 +2630,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t/* write the rest of the config */\n \t\tif (copy_begin < contents_sz)\n \t\t\tif (write_in_full(fd, contents + copy_begin,\n-\t\t\t\t\t contents_sz - copy_begin) <\n-\t\t\t contents_sz - copy_begin)\n+\t\t\t\t\t contents_sz - copy_begin) < 0)\n \t\t\t\tgoto write_err_out;\n \n \t\tmunmap(contents, contents_sz);\n-- \n2.14.1.874.ge7b2e05270\n\n"},{"id":"327950","messageId":"20170913171127.ocnitq6wz57ivhrh@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913170807.cyx7rrpoyhaauvol@sigill.intra.peff.net","subject":"[PATCH 2/7] get-tar-commit-id: check write_in_full() return against 0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T17:11:28Z","receivedAt":"2017-09-13T17:11:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We ask to write 41 bytes and make sure that the return value\nis at least 41. This is the same \"dangerous\" pattern that\nwas fixed in the prior commit (wherein a negative return\nvalue is promoted to unsigned), though it is not dangerous\nhere because our \"41\" is a constant, not an unsigned\nvariable.\n\nBut we should convert it anyway to avoid modeling a\ndangerous construct.\n\nSigned-off-by: Jeff King \n---\n builtin/get-tar-commit-id.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c\nindex e21c5416cd..6d9a79f9b3 100644\n--- a/builtin/get-tar-commit-id.c\n+++ b/builtin/get-tar-commit-id.c\n@@ -33,8 +33,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix)\n \tif (!skip_prefix(content, \"52 comment=\", &comment))\n \t\treturn 1;\n \n-\tn = write_in_full(1, comment, 41);\n-\tif (n < 41)\n+\tif (write_in_full(1, comment, 41) < 0)\n \t\tdie_errno(\"git get-tar-commit-id: write error\");\n \n \treturn 0;\n-- \n2.14.1.874.ge7b2e05270\n\n"},{"id":"327951","messageId":"20170913171603.3whuql7l5da3ynrn@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913170807.cyx7rrpoyhaauvol@sigill.intra.peff.net","subject":"[PATCH 3/7] avoid \"write_in_full(fd, buf, len) != len\" pattern","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T17:16:03Z","receivedAt":"2017-09-13T17:16:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The return value of write_in_full() is either \"-1\", or the\nrequested number of bytes[1]. If we make a partial write\nbefore seeing an error, we still return -1, not a partial\nvalue. This goes back to f6aa66cb95 (write_in_full: really\nwrite in full or return error on disk full., 2007-01-11).\n\nSo checking anything except \"was the return value negative\"\nis pointless. And there are a couple of reasons not to do\nso:\n\n 1. It can do a funny signed/unsigned comparison. If your\n \"len\" is signed (e.g., a size_t) then the compiler will\n promote the \"-1\" to its unsigned variant.\n\n This works out for \"!= len\" (unless you really were\n trying to write the maximum size_t bytes), but is a\n bug if you check \"< len\" (an example of which was fixed\n recently in config.c).\n\n We should avoid promoting the mental model that you\n need to check the length at all, so that new sites are\n not tempted to copy us.\n\n 2. Checking for a negative value is shorter to type,\n especially when the length is an expression.\n\n 3. Linus says so. In d34cf19b89 (Clean up write_in_full()\n users, 2007-01-11), right after the write_in_full()\n semantics were changed, he wrote:\n\n I really wish every \"write_in_full()\" user would just\n check against \"<0\" now, but this fixes the nasty and\n stupid ones.\n\n Appeals to authority aside, this makes it clear that\n writing it this way does not have an intentional\n benefit. It's a historical curiosity that we never\n bothered to clean up (and which was undoubtedly\n cargo-culted into new sites).\n\nSo let's convert these obviously-correct cases (this\nincludes write_str_in_full(), which is just a wrapper for\nwrite_in_full()).\n\n[1] A careful reader may notice there is one way that\n write_in_full() can return a different value. If we ask\n write() to write N bytes and get a return value that is\n _larger_ than N, we could return a larger total. But\n besides the fact that this would imply a totally broken\n version of write(), it would already invoke undefined\n behavior. Our internal remaining counter is an unsigned\n size_t, which means that subtracting too many byte will\n wrap it around to a very large number. So we'll instantly\n begin reading off the end of the buffer, trying to write\n gigabytes (or petabytes) of data.\n\nSigned-off-by: Jeff King \n---\n builtin/receive-pack.c | 2 +-\n builtin/rerere.c | 2 +-\n builtin/unpack-file.c | 2 +-\n config.c | 4 ++--\n diff.c | 2 +-\n fast-import.c | 2 +-\n http-backend.c | 4 ++--\n ll-merge.c | 2 +-\n read-cache.c | 6 +++---\n refs.c | 2 +-\n refs/files-backend.c | 8 ++++----\n rerere.c | 2 +-\n shallow.c | 6 +++---\n t/helper/test-delta.c | 2 +-\n transport-helper.c | 5 ++---\n wrapper.c | 2 +-\n 16 files changed, 26 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 52c63ebfdc..dd06b3fb4f 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -743,7 +743,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n \t\tsize_t n;\n \t\tif (feed(feed_state, &buf, &n))\n \t\t\tbreak;\n-\t\tif (write_in_full(proc.in, buf, n) != n)\n+\t\tif (write_in_full(proc.in, buf, n) < 0)\n \t\t\tbreak;\n \t}\n \tclose(proc.in);\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex ffb66e2907..0bc40298c2 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -18,7 +18,7 @@ static int outf(void *dummy, mmbuffer_t *ptr, int nbuf)\n {\n \tint i;\n \tfor (i = 0; i < nbuf; i++)\n-\t\tif (write_in_full(1, ptr[i].ptr, ptr[i].size) != ptr[i].size)\n+\t\tif (write_in_full(1, ptr[i].ptr, ptr[i].size) < 0)\n \t\t\treturn -1;\n \treturn 0;\n }\ndiff --git a/builtin/unpack-file.c b/builtin/unpack-file.c\nindex 281ca1db6c..32e0155577 100644\n--- a/builtin/unpack-file.c\n+++ b/builtin/unpack-file.c\n@@ -15,7 +15,7 @@ static char *create_temp_file(struct object_id *oid)\n \n \txsnprintf(path, sizeof(path), \".merge_file_XXXXXX\");\n \tfd = xmkstemp(path);\n-\tif (write_in_full(fd, buf, size) != size)\n+\tif (write_in_full(fd, buf, size) < 0)\n \t\tdie_errno(\"unable to write temp-file\");\n \tclose(fd);\n \treturn path;\ndiff --git a/config.c b/config.c\nindex eee4ac0355..daf093db45 100644\n--- a/config.c\n+++ b/config.c\n@@ -2611,7 +2611,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\t\t\t\t copy_end - copy_begin) < 0)\n \t\t\t\t\tgoto write_err_out;\n \t\t\t\tif (new_line &&\n-\t\t\t\t write_str_in_full(fd, \"\\n\") != 1)\n+\t\t\t\t write_str_in_full(fd, \"\\n\") < 0)\n \t\t\t\t\tgoto write_err_out;\n \t\t\t}\n \t\t\tcopy_begin = store.offset[i];\n@@ -2842,7 +2842,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n \t\tif (remove)\n \t\t\tcontinue;\n \t\tlength = strlen(output);\n-\t\tif (write_in_full(out_fd, output, length) != length) {\n+\t\tif (write_in_full(out_fd, output, length) < 0) {\n \t\t\tret = write_error(get_lock_file_path(lock));\n \t\t\tgoto out;\n \t\t}\ndiff --git a/diff.c b/diff.c\nindex 3d3e553a98..3914830ff1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3738,7 +3738,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,\n \t\tblob = buf.buf;\n \t\tsize = buf.len;\n \t}\n-\tif (write_in_full(fd, blob, size) != size)\n+\tif (write_in_full(fd, blob, size) < 0)\n \t\tdie_errno(\"unable to write temp-file\");\n \tclose_tempfile(&temp->tempfile);\n \ttemp->name = get_tempfile_path(&temp->tempfile);\ndiff --git a/fast-import.c b/fast-import.c\nindex 49516d60e6..35bf671f12 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2952,7 +2952,7 @@ static void parse_reset_branch(const char *arg)\n \n static void cat_blob_write(const char *buf, unsigned long size)\n {\n-\tif (write_in_full(cat_blob_fd, buf, size) != size)\n+\tif (write_in_full(cat_blob_fd, buf, size) < 0)\n \t\tdie_errno(\"Write to frontend failed\");\n }\n \ndiff --git a/http-backend.c b/http-backend.c\nindex 8076b1d5e5..e51c7805c8 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -358,7 +358,7 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)\n \t\t\t\tdie(\"zlib error inflating request, result %d\", ret);\n \n \t\t\tn = stream.total_out - cnt;\n-\t\t\tif (write_in_full(out, out_buf, n) != n)\n+\t\t\tif (write_in_full(out, out_buf, n) < 0)\n \t\t\t\tdie(\"%s aborted reading request\", prog_name);\n \t\t\tcnt += n;\n \n@@ -379,7 +379,7 @@ static void copy_request(const char *prog_name, int out)\n \tssize_t n = read_request(0, &buf);\n \tif (n < 0)\n \t\tdie_errno(\"error reading request body\");\n-\tif (write_in_full(out, buf, n) != n)\n+\tif (write_in_full(out, buf, n) < 0)\n \t\tdie(\"%s aborted reading request\", prog_name);\n \tclose(out);\n \tfree(buf);\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 9fb855a900..a6ad2ec12d 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -154,7 +154,7 @@ static void create_temp(mmfile_t *src, char *path, size_t len)\n \n \txsnprintf(path, len, \".merge_file_XXXXXX\");\n \tfd = xmkstemp(path);\n-\tif (write_in_full(fd, src->ptr, src->size) != src->size)\n+\tif (write_in_full(fd, src->ptr, src->size) < 0)\n \t\tdie_errno(\"unable to write temp-file\");\n \tclose(fd);\n }\ndiff --git a/read-cache.c b/read-cache.c\nindex 40da87ea71..357cea7a15 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1922,7 +1922,7 @@ static int ce_write_flush(git_SHA_CTX *context, int fd)\n \tunsigned int buffered = write_buffer_len;\n \tif (buffered) {\n \t\tgit_SHA1_Update(context, write_buffer, buffered);\n-\t\tif (write_in_full(fd, write_buffer, buffered) != buffered)\n+\t\tif (write_in_full(fd, write_buffer, buffered) < 0)\n \t\t\treturn -1;\n \t\twrite_buffer_len = 0;\n \t}\n@@ -1971,7 +1971,7 @@ static int ce_flush(git_SHA_CTX *context, int fd, unsigned char *sha1)\n \n \t/* Flush first if not enough space for SHA1 signature */\n \tif (left + 20 > WRITE_BUFFER_SIZE) {\n-\t\tif (write_in_full(fd, write_buffer, left) != left)\n+\t\tif (write_in_full(fd, write_buffer, left) < 0)\n \t\t\treturn -1;\n \t\tleft = 0;\n \t}\n@@ -1980,7 +1980,7 @@ static int ce_flush(git_SHA_CTX *context, int fd, unsigned char *sha1)\n \tgit_SHA1_Final(write_buffer + left, context);\n \thashcpy(sha1, write_buffer + left);\n \tleft += 20;\n-\treturn (write_in_full(fd, write_buffer, left) != left) ? -1 : 0;\n+\treturn (write_in_full(fd, write_buffer, left) < 0) ? -1 : 0;\n }\n \n static void ce_smudge_racily_clean_entry(struct cache_entry *ce)\ndiff --git a/refs.c b/refs.c\nindex b0106b8162..30201cf0cb 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -627,7 +627,7 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,\n \t\t}\n \t}\n \n-\tif (write_in_full(fd, buf.buf, buf.len) != buf.len) {\n+\tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n \t\tstrbuf_addf(err, \"could not write to '%s'\", filename);\n \t\trollback_lock_file(&lock);\n \t\tgoto done;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex fccbc24ac4..f8b91fff3f 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1628,8 +1628,8 @@ static int write_ref_to_lockfile(struct ref_lock *lock,\n \t\treturn -1;\n \t}\n \tfd = get_lock_file_fd(lock->lk);\n-\tif (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||\n-\t write_in_full(fd, &term, 1) != 1 ||\n+\tif (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) < 0 ||\n+\t write_in_full(fd, &term, 1) < 0 ||\n \t close_ref(lock) < 0) {\n \t\tstrbuf_addf(err,\n \t\t\t \"couldn't write '%s'\", get_lock_file_path(lock->lk));\n@@ -2853,8 +2853,8 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \t\t\t\t\tstrerror(errno));\n \t\t} else if (update &&\n \t\t\t (write_in_full(get_lock_file_fd(lock->lk),\n-\t\t\t\toid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||\n-\t\t\t write_str_in_full(get_lock_file_fd(lock->lk), \"\\n\") != 1 ||\n+\t\t\t\toid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) < 0 ||\n+\t\t\t write_str_in_full(get_lock_file_fd(lock->lk), \"\\n\") < 0 ||\n \t\t\t close_ref(lock) < 0)) {\n \t\t\tstatus |= error(\"couldn't write %s\",\n \t\t\t\t\tget_lock_file_path(lock->lk));\ndiff --git a/rerere.c b/rerere.c\nindex d77235645e..1ce440f4bb 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -258,7 +258,7 @@ static int write_rr(struct string_list *rr, int out_fd)\n \t\t\t\t rerere_id_hex(id),\n \t\t\t\t rr->items[i].string, 0);\n \n-\t\tif (write_in_full(out_fd, buf.buf, buf.len) != buf.len)\n+\t\tif (write_in_full(out_fd, buf.buf, buf.len) < 0)\n \t\t\tdie(\"unable to write rerere record\");\n \n \t\tstrbuf_release(&buf);\ndiff --git a/shallow.c b/shallow.c\nindex f5591e56da..57b494ab58 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -296,7 +296,7 @@ const char *setup_temporary_shallow(const struct oid_array *extra)\n \tif (write_shallow_commits(&sb, 0, extra)) {\n \t\tfd = xmks_tempfile(&temporary_shallow, git_path(\"shallow_XXXXXX\"));\n \n-\t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n+\t\tif (write_in_full(fd, sb.buf, sb.len) < 0)\n \t\t\tdie_errno(\"failed to write to %s\",\n \t\t\t\t get_tempfile_path(&temporary_shallow));\n \t\tclose_tempfile(&temporary_shallow);\n@@ -321,7 +321,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \tcheck_shallow_file_for_update();\n \tif (write_shallow_commits(&sb, 0, extra)) {\n-\t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n+\t\tif (write_in_full(fd, sb.buf, sb.len) < 0)\n \t\t\tdie_errno(\"failed to write to %s\",\n \t\t\t\t get_lock_file_path(shallow_lock));\n \t\t*alternate_shallow_file = get_lock_file_path(shallow_lock);\n@@ -368,7 +368,7 @@ void prune_shallow(int show_only)\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \tcheck_shallow_file_for_update();\n \tif (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {\n-\t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n+\t\tif (write_in_full(fd, sb.buf, sb.len) < 0)\n \t\t\tdie_errno(\"failed to write to %s\",\n \t\t\t\t get_lock_file_path(&shallow_lock));\n \t\tcommit_lock_file(&shallow_lock);\ndiff --git a/t/helper/test-delta.c b/t/helper/test-delta.c\nindex 59937dc1be..591730adc4 100644\n--- a/t/helper/test-delta.c\n+++ b/t/helper/test-delta.c\n@@ -69,7 +69,7 @@ int cmd_main(int argc, const char **argv)\n \t}\n \n \tfd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);\n-\tif (fd < 0 || write_in_full(fd, out_buf, out_size) != out_size) {\n+\tif (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {\n \t\tperror(argv[4]);\n \t\treturn 1;\n \t}\ndiff --git a/transport-helper.c b/transport-helper.c\nindex f50b34df2d..b66cb7d8e2 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -44,8 +44,7 @@ static void sendline(struct helper_data *helper, struct strbuf *buffer)\n {\n \tif (debug)\n \t\tfprintf(stderr, \"Debug: Remote helper: -> %s\", buffer->buf);\n-\tif (write_in_full(helper->helper->in, buffer->buf, buffer->len)\n-\t\t!= buffer->len)\n+\tif (write_in_full(helper->helper->in, buffer->buf, buffer->len) < 0)\n \t\tdie_errno(\"Full write to remote helper failed\");\n }\n \n@@ -74,7 +73,7 @@ static void write_constant(int fd, const char *str)\n {\n \tif (debug)\n \t\tfprintf(stderr, \"Debug: Remote helper: -> %s\", str);\n-\tif (write_in_full(fd, str, strlen(str)) != strlen(str))\n+\tif (write_in_full(fd, str, strlen(str)) < 0)\n \t\tdie_errno(\"Full write to remote helper failed\");\n }\n \ndiff --git a/wrapper.c b/wrapper.c\nindex 36630e5d18..61aba0b5c1 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -652,7 +652,7 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...)\n void write_file_buf(const char *path, const char *buf, size_t len)\n {\n \tint fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n-\tif (write_in_full(fd, buf, len) != len)\n+\tif (write_in_full(fd, buf, len) < 0)\n \t\tdie_errno(_(\"could not write to %s\"), path);\n \tif (close(fd))\n \t\tdie_errno(_(\"could not close %s\"), path);\n-- \n2.14.1.874.ge7b2e05270\n\n"},{"id":"327957","messageId":"20170913171628.o4euxlzk4imlf4tv@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913170807.cyx7rrpoyhaauvol@sigill.intra.peff.net","subject":"[PATCH 4/7] convert less-trivial versions of \"write_in_full() != len\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T17:16:28Z","receivedAt":"2017-09-13T17:16:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The prior commit converted many sites to check the return\nvalue of write_in_full() for negativity, rather than a\nmismatch with the input length. This patch covers similar\ncases, but where the return value is stored in an\nintermediate variable. These should get the same treatment,\nbut they need to be reviewed more carefully since it would\nbe a bug if the return value is stored in an unsigned type\n(which indeed, it is in one of the cases).\n\nSigned-off-by: Jeff King \n---\n entry.c | 5 +++--\n refs/files-backend.c | 2 +-\n streaming.c | 2 +-\n 3 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex cb291aa88b..1c7e3c11d5 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -257,7 +257,8 @@ static int write_entry(struct cache_entry *ce,\n \tchar *new;\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned long size;\n-\tsize_t wrote, newsize = 0;\n+\tssize_t wrote;\n+\tsize_t newsize = 0;\n \tstruct stat st;\n \tconst struct submodule *sub;\n \n@@ -332,7 +333,7 @@ static int write_entry(struct cache_entry *ce,\n \t\t\tfstat_done = fstat_output(fd, state, &st);\n \t\tclose(fd);\n \t\tfree(new);\n-\t\tif (wrote != size)\n+\t\tif (wrote < 0)\n \t\t\treturn error(\"unable to write file %s\", path);\n \t\tbreak;\n \tcase S_IFGITLINK:\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f8b91fff3f..489471bbcf 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1549,7 +1549,7 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,\n \n \twritten = len <= maxlen ? write_in_full(fd, logrec, len) : -1;\n \tfree(logrec);\n-\tif (written != len)\n+\tif (written < 0)\n \t\treturn -1;\n \n \treturn 0;\ndiff --git a/streaming.c b/streaming.c\nindex 6f1c60f12b..5892b50bd8 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -540,7 +540,7 @@ int stream_blob_to_fd(int fd, const struct object_id *oid, struct stream_filter\n \t\t\tkept = 0;\n \t\twrote = write_in_full(fd, buf, readlen);\n \n-\t\tif (wrote != readlen)\n+\t\tif (wrote < 0)\n \t\t\tgoto close_and_exit;\n \t}\n \tif (kept && (lseek(fd, kept - 1, SEEK_CUR) == (off_t) -1 ||\n-- \n2.14.1.874.ge7b2e05270\n\n"},{"id":"327971","messageId":"20170913171730.ckcebchjhhvtwcsj@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913170807.cyx7rrpoyhaauvol@sigill.intra.peff.net","subject":"[PATCH 5/7] pkt-line: check write_in_full() errors against \"< 0\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T17:17:30Z","receivedAt":"2017-09-13T17:17:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As with the previous two commits, we prefer to check\nwrite_in_full()'s return value to see if it is negative,\nrather than comparing it to the input length.\n\nThese cases actually flip the logic to check for success,\nmaking conversion a little different than in other cases. We\ncould of course write:\n\n if (write_in_full(...) >= 0)\n return 0;\n return error(...);\n\nBut our usual method of spelling write() error checks is\njust \"< 0\". So let's flip the logic for each of these\nconditionals to our usual style.\n\nSigned-off-by: Jeff King \n---\nThis will have a minor textual conflict with ma/pkt-line-leakfix which\nhasn't quite made it into master yet. The resolution should be\nstraight-forward, though.\n\n pkt-line.c | 29 ++++++++++++++---------------\n 1 file changed, 14 insertions(+), 15 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 7db9119573..4823d3bb9d 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -94,9 +94,9 @@ void packet_flush(int fd)\n int packet_flush_gently(int fd)\n {\n \tpacket_trace(\"0000\", 4, 1);\n-\tif (write_in_full(fd, \"0000\", 4) == 4)\n-\t\treturn 0;\n-\treturn error(\"flush packet write failed\");\n+\tif (write_in_full(fd, \"0000\", 4) < 0)\n+\t\treturn error(\"flush packet write failed\");\n+\treturn 0;\n }\n \n void packet_buf_flush(struct strbuf *buf)\n@@ -137,18 +137,17 @@ static int packet_write_fmt_1(int fd, int gently,\n \t\t\t const char *fmt, va_list args)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tssize_t count;\n \n \tformat_packet(&buf, fmt, args);\n-\tcount = write_in_full(fd, buf.buf, buf.len);\n-\tif (count == buf.len)\n-\t\treturn 0;\n-\n-\tif (!gently) {\n-\t\tcheck_pipe(errno);\n-\t\tdie_errno(\"packet write with format failed\");\n+\tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n+\t\tif (!gently) {\n+\t\t\tcheck_pipe(errno);\n+\t\t\tdie_errno(\"packet write with format failed\");\n+\t\t}\n+\t\treturn error(\"packet write with format failed\");\n \t}\n-\treturn error(\"packet write with format failed\");\n+\n+\treturn 0;\n }\n \n void packet_write_fmt(int fd, const char *fmt, ...)\n@@ -183,9 +182,9 @@ static int packet_write_gently(const int fd_out, const char *buf, size_t size)\n \tpacket_size = size + 4;\n \tset_packet_header(packet_write_buffer, packet_size);\n \tmemcpy(packet_write_buffer + 4, buf, size);\n-\tif (write_in_full(fd_out, packet_write_buffer, packet_size) == packet_size)\n-\t\treturn 0;\n-\treturn error(\"packet write failed\");\n+\tif (write_in_full(fd_out, packet_write_buffer, packet_size) < 0)\n+\t\treturn error(\"packet write failed\");\n+\treturn 0;\n }\n \n void packet_buf_write(struct strbuf *buf, const char *fmt, ...)\n-- \n2.14.1.874.ge7b2e05270\n\n"},{"id":"327975","messageId":"20170913171744.32cmtjkdcp7yf7pt@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913170807.cyx7rrpoyhaauvol@sigill.intra.peff.net","subject":"[PATCH 6/7] notes-merge: use ssize_t for write_in_full() return value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T17:17:44Z","receivedAt":"2017-09-13T17:17:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We store the return value of write_in_full() in a long,\nthough the return is actually an ssize_t. This probably\ndoesn't matter much in practice (since the buffer size is\nalredy an unsigned long), but it might if the size if\nbetween what can be represented in \"long\" and \"unsigned\nlong\", and if your size_t is larger than a \"long\" (as it is\non 64-bit Windows).\n\nSigned-off-by: Jeff King \n---\n notes-merge.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/notes-merge.c b/notes-merge.c\nindex b04d2f2131..597d43f65c 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -302,7 +302,7 @@ static void write_buf_to_worktree(const struct object_id *obj,\n \tfd = xopen(path, O_WRONLY | O_EXCL | O_CREAT, 0666);\n \n \twhile (size > 0) {\n-\t\tlong ret = write_in_full(fd, buf, size);\n+\t\tssize_t ret = write_in_full(fd, buf, size);\n \t\tif (ret < 0) {\n \t\t\t/* Ignore epipe */\n \t\t\tif (errno == EPIPE)\n-- \n2.14.1.874.ge7b2e05270\n\n"},{"id":"327976","messageId":"20170913171756.v7vzu3a77g2khq7x@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913170807.cyx7rrpoyhaauvol@sigill.intra.peff.net","subject":"[PATCH 7/7] config: flip return value of store_write_*()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T17:17:57Z","receivedAt":"2017-09-13T17:18:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The store_write_section() and store_write_pairs() functions\nare basically high-level wrappers around write(). But their\nreturn values are flipped from our usual convention, using\n\"1\" for success and \"0\" for failure.\n\nLet's flip them to follow the usual write() conventions and\nupdate all callers. As these are local to config.c, it's\nunlikely that we'd have new callers in any topics in flight\n(which would be silently broken by our change). But just to\nbe on the safe side, let's rename them to just\nwrite_section() and write_pairs(). That also accentuates\ntheir relationship with write().\n\nSigned-off-by: Jeff King \n---\n config.c | 28 +++++++++++++++-------------\n 1 file changed, 15 insertions(+), 13 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex daf093db45..f8b7b81445 100644\n--- a/config.c\n+++ b/config.c\n@@ -2297,10 +2297,11 @@ static int write_error(const char *filename)\n \treturn 4;\n }\n \n-static int store_write_section(int fd, const char *key)\n+static ssize_t write_section(int fd, const char *key)\n {\n \tconst char *dot;\n-\tint i, success;\n+\tint i;\n+\tssize_t ret;\n \tstruct strbuf sb = STRBUF_INIT;\n \n \tdot = memchr(key, '.', store.baselen);\n@@ -2316,15 +2317,16 @@ static int store_write_section(int fd, const char *key)\n \t\tstrbuf_addf(&sb, \"[%.*s]\\n\", store.baselen, key);\n \t}\n \n-\tsuccess = write_in_full(fd, sb.buf, sb.len) == sb.len;\n+\tret = write_in_full(fd, sb.buf, sb.len);\n \tstrbuf_release(&sb);\n \n-\treturn success;\n+\treturn ret;\n }\n \n-static int store_write_pair(int fd, const char *key, const char *value)\n+static ssize_t write_pair(int fd, const char *key, const char *value)\n {\n-\tint i, success;\n+\tint i;\n+\tssize_t ret;\n \tint length = strlen(key + store.baselen + 1);\n \tconst char *quote = \"\";\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -2364,10 +2366,10 @@ static int store_write_pair(int fd, const char *key, const char *value)\n \t\t}\n \tstrbuf_addf(&sb, \"%s\\n\", quote);\n \n-\tsuccess = write_in_full(fd, sb.buf, sb.len) == sb.len;\n+\tret = write_in_full(fd, sb.buf, sb.len);\n \tstrbuf_release(&sb);\n \n-\treturn success;\n+\treturn ret;\n }\n \n static ssize_t find_beginning_of_line(const char *contents, size_t size,\n@@ -2497,8 +2499,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t}\n \n \t\tstore.key = (char *)key;\n-\t\tif (!store_write_section(fd, key) ||\n-\t\t !store_write_pair(fd, key, value))\n+\t\tif (write_section(fd, key) < 0 ||\n+\t\t write_pair(fd, key, value) < 0)\n \t\t\tgoto write_err_out;\n \t} else {\n \t\tstruct stat st;\n@@ -2620,10 +2622,10 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t/* write the pair (value == NULL means unset) */\n \t\tif (value != NULL) {\n \t\t\tif (store.state == START) {\n-\t\t\t\tif (!store_write_section(fd, key))\n+\t\t\t\tif (write_section(fd, key) < 0)\n \t\t\t\t\tgoto write_err_out;\n \t\t\t}\n-\t\t\tif (!store_write_pair(fd, key, value))\n+\t\t\tif (write_pair(fd, key, value) < 0)\n \t\t\t\tgoto write_err_out;\n \t\t}\n \n@@ -2816,7 +2818,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n \t\t\t\tstore.baselen = strlen(new_name);\n-\t\t\t\tif (!store_write_section(out_fd, new_name)) {\n+\t\t\t\tif (write_section(out_fd, new_name) < 0) {\n \t\t\t\t\tret = write_error(get_lock_file_path(lock));\n \t\t\t\t\tgoto out;\n \t\t\t\t}\n-- \n2.14.1.874.ge7b2e05270\n"},{"id":"327977","messageId":"20170913174728.GB27425@aiede.mtv.corp.google.com","threadId":"46742","inReplyTo":"20170913171104.yu7ags4aq2zdwz6r@sigill.intra.peff.net","subject":"Re: [PATCH 1/7] config: avoid \"write_in_full(fd, buf, len) < len\" pattern","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-13T17:47:28Z","receivedAt":"2017-09-13T17:47:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> I scoured the code base for cases of this, but it turns out\n> that these two in git_config_set_multivar_in_file_gently()\n> are the only ones. This case is actually quite interesting:\n> we don't have a size_t, but rather use the subtraction of\n> two pointers. Which you might think would be a signed\n> ptrdiff_t, but clearly both gcc and clang treat it as\n> unsigned (possibly because the conditional just above\n> guarantees that the result is greater than zero).\n\nDo you have more detail about this? I get worried when I read\nsomething like this that sounds like a compiler bug.\n\nC99 sayeth:\n\n\tWhen two pointers are subtracted, both shall point to elements\n\tof the same array object, or one past the last element of the\n\tarray object; the result is the difference of the subscripts\n\tof the two array elements. The size of the result is\n\timplementation-defined, and its type (a signed integer type)\n\tis ptrdiff_t defined in the header.\n\nHow can I reproduce the problem?\n\n> We can avoid the whole question by just checking for a\n> negative return value directly, as write_in_full() will\n> never return any value except -1 or the full count.\n>\n> There's no addition to the test suite here, since you need\n> to convince write() to fail in order to see the problem. The\n> simplest reproduction recipe I came up with is to trigger\n> ENOSPC (this only works on Linux, obviously):\n\nDoes /dev/full make it simpler to reproduce?\n\nThanks,\nJonathan\n"},{"id":"327978","messageId":"20170913175338.tsq4hmgmmybp43dw@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913174728.GB27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/7] config: avoid \"write_in_full(fd, buf, len) < len\" pattern","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T17:53:38Z","receivedAt":"2017-09-13T17:53:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2017 at 10:47:28AM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > I scoured the code base for cases of this, but it turns out\n> > that these two in git_config_set_multivar_in_file_gently()\n> > are the only ones. This case is actually quite interesting:\n> > we don't have a size_t, but rather use the subtraction of\n> > two pointers. Which you might think would be a signed\n> > ptrdiff_t, but clearly both gcc and clang treat it as\n> > unsigned (possibly because the conditional just above\n> > guarantees that the result is greater than zero).\n> \n> Do you have more detail about this? I get worried when I read\n> something like this that sounds like a compiler bug.\n> \n> C99 sayeth:\n> \n> \tWhen two pointers are subtracted, both shall point to elements\n> \tof the same array object, or one past the last element of the\n> \tarray object; the result is the difference of the subscripts\n> \tof the two array elements. The size of the result is\n> \timplementation-defined, and its type (a signed integer type)\n> \tis ptrdiff_t defined in the header.\n\nI'm not sure if it's a compiler bug or not. I read the bits about\nptrdiff_t, and it wasn't entirely clear to me if a pointer difference\n_is_ an actual ptrdiff_t, or if it can generally be stored in one. Right\nbelow that text it also says:\n\n If the result is not representable in an object of that type, the\n behavior is undefined.\n\nThat said, I might be wrong that unsigned promotion is the culprit. I\ndidn't look at the generated assembly. But I also can't see what else\nwould be causing the problem here. We're clearly returning \"-1\" and the\ncondition doesn't trigger.\n\n> How can I reproduce the problem?\n\nI gave a recipe in the commit message, which is the best I came up with.\nYou could probably use a fault-injection library to convince write() to\nfail. Or just tweak the source code to have write_in_full() return -1.\n\n> > There's no addition to the test suite here, since you need\n> > to convince write() to fail in order to see the problem. The\n> > simplest reproduction recipe I came up with is to trigger\n> > ENOSPC (this only works on Linux, obviously):\n> \n> Does /dev/full make it simpler to reproduce?\n\nI don't think so, because the write() failure is to the lockfile, which\nis created with O_EXCL. So even if you could convince \"config.lock\" to\nbe the right device type, the open() would fail.\n\n-Peff\n"},{"id":"327979","messageId":"20170913175357.GC27425@aiede.mtv.corp.google.com","threadId":"46742","inReplyTo":"20170913171127.ocnitq6wz57ivhrh@sigill.intra.peff.net","subject":"Re: [PATCH 2/7] get-tar-commit-id: check write_in_full() return against 0","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-13T17:53:57Z","receivedAt":"2017-09-13T17:54:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> We ask to write 41 bytes and make sure that the return value\n> is at least 41. This is the same \"dangerous\" pattern that\n> was fixed in the prior commit (wherein a negative return\n> value is promoted to unsigned), though it is not dangerous\n> here because our \"41\" is a constant, not an unsigned\n> variable.\n>\n> But we should convert it anyway to avoid modeling a\n> dangerous construct.\n\nIf the above logic is correct, then I suspect this series does not go\nfar enough. write_in_full() would be one of those APIs that is easy\nto misuse and difficult to use correctly, and if so we should fix that\nat the source instead of trying to teach callers not to hold it wrong.\n\nE.g. what would you think of\n\n 1. Introduce a write_fully (sorry, I am bad at names) function\n that returns 0 on success and a coccinelle semantic patch in\n contrib/coccinelle to migrate callers in \"make coccicheck\":\n\n@@\nexpression E;\nexpression F;\nexpression G;\n@@\n-write_in_full(E, F, G) < G\n+write_fully(E, F, G)\n\n 2. Run \"make coccicheck\" and apply the result.\n 3. Remove the write_in_full function.\n\nDoes read_in_full need a similar treatment?\n\nThanks,\nJonathan\n"},{"id":"327980","messageId":"20170913175930.GD27425@aiede.mtv.corp.google.com","threadId":"46742","inReplyTo":"20170913175338.tsq4hmgmmybp43dw@sigill.intra.peff.net","subject":"Re: [PATCH 1/7] config: avoid \"write_in_full(fd, buf, len) < len\" pattern","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-13T17:59:30Z","receivedAt":"2017-09-13T17:59:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Wed, Sep 13, 2017 at 10:47:28AM -0700, Jonathan Nieder wrote:\n>> Jeff King wrote:\n\n>>> I scoured the code base for cases of this, but it turns out\n>>> that these two in git_config_set_multivar_in_file_gently()\n>>> are the only ones. This case is actually quite interesting:\n>>> we don't have a size_t, but rather use the subtraction of\n>>> two pointers. Which you might think would be a signed\n>>> ptrdiff_t, but clearly both gcc and clang treat it as\n>>> unsigned (possibly because the conditional just above\n>>> guarantees that the result is greater than zero).\n>>\n>> Do you have more detail about this? I get worried when I read\n>> something like this that sounds like a compiler bug.\n>>\n>> C99 sayeth:\n>>\n>> \tWhen two pointers are subtracted, both shall point to elements\n>> \tof the same array object, or one past the last element of the\n>> \tarray object; the result is the difference of the subscripts\n>> \tof the two array elements. The size of the result is\n>> \timplementation-defined, and its type (a signed integer type)\n>> \tis ptrdiff_t defined in the header.\n>\n> I'm not sure if it's a compiler bug or not. I read the bits about\n> ptrdiff_t, and it wasn't entirely clear to me if a pointer difference\n> _is_ an actual ptrdiff_t, or if it can generally be stored in one. Right\n> below that text it also says:\n>\n> If the result is not representable in an object of that type, the\n> behavior is undefined.\n\nI can confidentally say the intent in C99 in that passage is to\ndescribe the type of the expression, not just the type of a variable\nthat can hold it.\n\n> That said, I might be wrong that unsigned promotion is the culprit. I\n> didn't look at the generated assembly. But I also can't see what else\n> would be causing the problem here. We're clearly returning \"-1\" and the\n> condition doesn't trigger.\n>\n>> How can I reproduce the problem?\n>\n> I gave a recipe in the commit message, which is the best I came up with.\n> You could probably use a fault-injection library to convince write() to\n> fail. Or just tweak the source code to have write_in_full() return -1.\n\nI wonder if a new test helper in t/helper/ would be able to do it (since\nthen it could e.g. control the filename that write_in_full writes to).\n\n>>> There's no addition to the test suite here, since you need\n>>> to convince write() to fail in order to see the problem. The\n>>> simplest reproduction recipe I came up with is to trigger\n>>> ENOSPC (this only works on Linux, obviously):\n>>\n>> Does /dev/full make it simpler to reproduce?\n>\n> I don't think so, because the write() failure is to the lockfile, which\n> is created with O_EXCL. So even if you could convince \"config.lock\" to\n> be the right device type, the open() would fail.\n\nHm, you're convincing me that it would indeed be worth hooking into a\nfault injection framework (that e.g. uses LD_PRELOAD), but that's a\ntopic for another day.\n\nThanks,\nJonathan\n"},{"id":"327981","messageId":"20170913180232.luxcnx7fbcpbt7bw@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913175357.GC27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH 2/7] get-tar-commit-id: check write_in_full() return against 0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T18:02:33Z","receivedAt":"2017-09-13T18:02:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2017 at 10:53:57AM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > We ask to write 41 bytes and make sure that the return value\n> > is at least 41. This is the same \"dangerous\" pattern that\n> > was fixed in the prior commit (wherein a negative return\n> > value is promoted to unsigned), though it is not dangerous\n> > here because our \"41\" is a constant, not an unsigned\n> > variable.\n> >\n> > But we should convert it anyway to avoid modeling a\n> > dangerous construct.\n> \n> If the above logic is correct, then I suspect this series does not go\n> far enough. write_in_full() would be one of those APIs that is easy\n> to misuse and difficult to use correctly, and if so we should fix that\n> at the source instead of trying to teach callers not to hold it wrong.\n\nYes, this series is just removing bad examples. It doesn't do anything\nto make write_in_full() less potentially dangerous.\n\nOn the other hand, it's no more or less dangerous than write(), which\nhas the same return-value semantics.\n\n> E.g. what would you think of\n> \n> 1. Introduce a write_fully (sorry, I am bad at names) function\n> that returns 0 on success and a coccinelle semantic patch in\n> contrib/coccinelle to migrate callers in \"make coccicheck\":\n\nYes, I considered that, though some callers really do care about\nassigning the number of bytes written. The fact that write() has the\nsame problem, plus the fact that there were only 2 buggy instances\nacross the code base made me think there's not a huge gain to that extra\nstep.\n\n> @@\n> expression E;\n> expression F;\n> expression G;\n> @@\n> -write_in_full(E, F, G) < G\n> +write_fully(E, F, G)\n> \n> 2. Run \"make coccicheck\" and apply the result.\n> 3. Remove the write_in_full function.\n\nThere's a step between those where you have to update all of the\nwrite_in_full() callers that store the result. Some of them would be\ntrivial conversions, but some of them actually care about the length\nE.g., the one in imap-send.c, which is the only one I didn't convert\naway from \"!= len\" because it's half of an #ifdef with SSL_write()\n(which uses an \"int\" as the return value!).\n\n> Does read_in_full need a similar treatment?\n\nIt might actually return fewer than the requested number of bytes, so it\ncan't just use \"< 0\" in the same way (nor be adapted to return 0 on\nsuccess). But I think it's still a bug to do:\n\n char buf[20];\n size_t len = sizeof(buf);\n if (read_in_full(fd, buf, len) < len)\n die(...);\n\nsince that will promote the -1 to a size_t. So it's probably worth\nauditing.\n\n-Peff\n"},{"id":"327982","messageId":"20170913181127.rl2kfqkw7basp6kb@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913175930.GD27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/7] config: avoid \"write_in_full(fd, buf, len) < len\" pattern","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T18:11:27Z","receivedAt":"2017-09-13T18:11:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2017 at 10:59:30AM -0700, Jonathan Nieder wrote:\n\n> I can confidentally say the intent in C99 in that passage is to\n> describe the type of the expression, not just the type of a variable\n> that can hold it.\n\nDoh. The problem is that I'm a moron. The copy_begin and copy_end values\nare _not_ pointers, they're size_t. That's why we have to use:\n\n contents + copy_begin\n\nas the buffer. So there is no ptrdiff_t involved at all, just a size_t.\n\n-Peff\n"},{"id":"327983","messageId":"20170913181515.p7u2ouine3ysblqc@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913171104.yu7ags4aq2zdwz6r@sigill.intra.peff.net","subject":"[PATCH v2] config: avoid \"write_in_full(fd, buf, len) < len\" pattern","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T18:15:16Z","receivedAt":"2017-09-13T18:15:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2017 at 01:11:04PM -0400, Jeff King wrote:\n\n> I scoured the code base for cases of this, but it turns out\n> that these two in git_config_set_multivar_in_file_gently()\n> are the only ones. This case is actually quite interesting:\n> we don't have a size_t, but rather use the subtraction of\n> two pointers. Which you might think would be a signed\n> ptrdiff_t, but clearly both gcc and clang treat it as\n> unsigned (possibly because the conditional just above\n> guarantees that the result is greater than zero).\n\nSorry, this is totally wrong. As Jonathan pointed out (and as I was\npuzzled over here), the result of a pointer subtraction _is_ a signed\ntype, and works fine.\n\nWhat I missed is that copy_begin and copy_end here are actually size_t\nvariables, not the pointers. Sorry for the confusion, and here's an\nupdated version of the patch with this paragraph amended (the patch\nitself is identical):\n\n-- >8 --\nSubject: [PATCH] config: avoid \"write_in_full(fd, buf, len) < len\" pattern\n\nThe return type of write_in_full() is a signed ssize_t,\nbecause we may return \"-1\" on failure (even if we succeeded\nin writing some bytes). But \"len\" itself is may be an\nunsigned type (the function takes a size_t, but of course we\nmay have something else in the calling function). So while\nit seems like:\n\n if (write_in_full(fd, buf, len) < len)\n\tdie_errno(\"write error\");\n\nwould trigger on error, it won't if \"len\" is unsigned. The\ncompiler sees a signed/unsigned comparison and promotes the\nsigned value, resulting in (size_t)-1, the highest possible\nsize_t (or again, whatever type the caller has). This cannot\npossibly be smaller than \"len\", and so the conditional can\nnever trigger.\n\nI scoured the code base for cases of this, but it turns out\nthat these two in git_config_set_multivar_in_file_gently()\nare the only ones. Here our \"len\" is the difference between\ntwo size_t variables, making the result an unsigned size_t.\nWe can fix this by just checking for a negative return value\ndirectly, as write_in_full() will never return any value\nexcept -1 or the full count.\n\nThere's no addition to the test suite here, since you need\nto convince write() to fail in order to see the problem. The\nsimplest reproduction recipe I came up with is to trigger\nENOSPC:\n\n # make a limited-size filesystem\n dd if=/dev/zero of=small.disk bs=1M count=1\n mke2fs small.disk\n mkdir mnt\n sudo mount -o loop small.disk mnt\n cd mnt\n sudo chown $USER:$USER .\n\n # make a config file with some content\n git config --file=config one.key value\n git config --file=config two.key value\n\n # now fill up the disk\n dd if=/dev/zero of=fill\n\n # and try to delete a key, which requires copying the rest\n # of the file to config.lock, and will fail on write()\n git config --file=config --unset two.key\n\nThat final command should (and does after this patch)\nproduce an error message due to the failed write, and leave\nthe file intact. Instead, it silently ignores the failure\nand renames config.lock into place, leaving you with a\ntotally empty config file!\n\nReported-by: demerphq \nSigned-off-by: Jeff King \n---\n config.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex d0d8ce823a..eee4ac0355 100644\n--- a/config.c\n+++ b/config.c\n@@ -2608,8 +2608,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\t/* write the first part of the config */\n \t\t\tif (copy_end > copy_begin) {\n \t\t\t\tif (write_in_full(fd, contents + copy_begin,\n-\t\t\t\t\t\t copy_end - copy_begin) <\n-\t\t\t\t copy_end - copy_begin)\n+\t\t\t\t\t\t copy_end - copy_begin) < 0)\n \t\t\t\t\tgoto write_err_out;\n \t\t\t\tif (new_line &&\n \t\t\t\t write_str_in_full(fd, \"\\n\") != 1)\n@@ -2631,8 +2630,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t/* write the rest of the config */\n \t\tif (copy_begin < contents_sz)\n \t\t\tif (write_in_full(fd, contents + copy_begin,\n-\t\t\t\t\t contents_sz - copy_begin) <\n-\t\t\t contents_sz - copy_begin)\n+\t\t\t\t\t contents_sz - copy_begin) < 0)\n \t\t\t\tgoto write_err_out;\n \n \t\tmunmap(contents, contents_sz);\n-- \n2.14.1.874.ge7b2e05270\n\n"},{"id":"327984","messageId":"20170913182431.GE27425@aiede.mtv.corp.google.com","threadId":"46742","inReplyTo":"20170913181515.p7u2ouine3ysblqc@sigill.intra.peff.net","subject":"Re: [PATCH v2] config: avoid \"write_in_full(fd, buf, len) < len\" pattern","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-13T18:24:31Z","receivedAt":"2017-09-13T18:24:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> What I missed is that copy_begin and copy_end here are actually size_t\n> variables, not the pointers. Sorry for the confusion, and here's an\n> updated version of the patch with this paragraph amended (the patch\n> itself is identical):\n\nSubtle. The world makes more sense now. Thanks for figuring it out.\n\n> -- >8 --\n> Subject: [PATCH] config: avoid \"write_in_full(fd, buf, len) < len\" pattern\n> \n> The return type of write_in_full() is a signed ssize_t,\n> because we may return \"-1\" on failure (even if we succeeded\n> in writing some bytes). But \"len\" itself is may be an\n> unsigned type (the function takes a size_t, but of course we\n> may have something else in the calling function). So while\n> it seems like:\n>\n> if (write_in_full(fd, buf, len) < len)\n> \tdie_errno(\"write error\");\n>\n> would trigger on error, it won't if \"len\" is unsigned. The\n> compiler sees a signed/unsigned comparison and promotes the\n> signed value, resulting in (size_t)-1, the highest possible\n> size_t (or again, whatever type the caller has). This cannot\n> possibly be smaller than \"len\", and so the conditional can\n> never trigger.\n>\n> I scoured the code base for cases of this, but it turns out\n> that these two in git_config_set_multivar_in_file_gently()\n> are the only ones. Here our \"len\" is the difference between\n> two size_t variables, making the result an unsigned size_t.\n> We can fix this by just checking for a negative return value\n> directly, as write_in_full() will never return any value\n> except -1 or the full count.\n[...]\n> Reported-by: demerphq \n> Signed-off-by: Jeff King \n> ---\n\nFor what it's worth,\nReviewed-by: Jonathan Nieder \n\nThank you.\n\nCompilers' signed/unsigned comparison warning can be noisy, but I'm\nstarting to feel it's worth the suppression noise to turn it on when\nDEVELOPER=1 anyway. What do you think? Is there a way to turn it on\nselectively for certain functions on the LHS (like read() and write()\nstyle functions)?\n\nSincerely,\nJonathan\n"},{"id":"327985","messageId":"20170913183700.amtrquhg66hjrbsp@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913180232.luxcnx7fbcpbt7bw@sigill.intra.peff.net","subject":"Re: [PATCH 2/7] get-tar-commit-id: check write_in_full() return against 0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T18:37:00Z","receivedAt":"2017-09-13T18:37:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2017 at 02:02:32PM -0400, Jeff King wrote:\n\n> > Does read_in_full need a similar treatment?\n> \n> It might actually return fewer than the requested number of bytes, so it\n> can't just use \"< 0\" in the same way (nor be adapted to return 0 on\n> success). But I think it's still a bug to do:\n> \n> char buf[20];\n> size_t len = sizeof(buf);\n> if (read_in_full(fd, buf, len) < len)\n> die(...);\n> \n> since that will promote the -1 to a size_t. So it's probably worth\n> auditing.\n\nI looked it over and found one potentially buggy case. In\nread-pack_header(), we do:\n\n if (read_in_full(fd, header, sizeof(*header)) < sizeof(*header))\n /* \"eof before pack header was fully read\" */\n return PH_ERROR_EOF;\n\nwhich will treat a read error as a silent success. I don't think it's\nharmful in practice because the next line checks that the header\nmatches the PACK signature (which yes, means we're reading uninitialized\nbytes, but the chances are 1 in 2^32 that they match the signature.\nUnless the buffer had an old PACK signature in it, of course ;) ).\n\nThere's one other harmless \"< len\" check. There are a bunch of \"!=\"\nchecks which are OK as-is. Converting them to something equally short is\nhard, though we could introduce a helper similar to your write_fully(),\nlike:\n\n int read_exactly(int fd, char *buf, size_t len)\n {\n\tssize_t ret = read_in_full(fd, buf, len);\n\tif (ret < 0)\n\t\treturn -1; /* real error */\n\telse if (ret < len)\n\t\treturn -1; /* early eof */\n\treturn 0;\n }\n\nBut the trouble is that a _good_ caller actually handles those cases\nseparately to provide better error messages (by doing that same\nif-cascade themselves). If those if's were turned into error() or die()\ncalls, then we'd actually be improving the callsites. But of course not\nall of them actually print an error or die. And when they do, they\nusually say something specific about _what_ they were trying to read.\n\nSo it may not be worth trying to do a mass-cleanup in the same way here.\n\n-Peff\n"},{"id":"327986","messageId":"20170913184722.imcexldnufskugqz@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913170807.cyx7rrpoyhaauvol@sigill.intra.peff.net","subject":"[PATCH 8/7] read_pack_header: handle signed/unsigned comparison in read result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T18:47:22Z","receivedAt":"2017-09-13T18:47:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2017 at 01:08:07PM -0400, Jeff King wrote:\n\n> Here's the fix, along with some related cleanups. The actual bugfix is\n> in the first patch, which I think should go to \"maint\". The rest are not\n> as important, so could go to master (but would also be fine for maint,\n> as there should be no user-visible changes).\n> \n> [1/7]: config: avoid \"write_in_full(fd, buf, len) < len\" pattern\n> [2/7]: get-tar-commit-id: check write_in_full() return against 0\n> [3/7]: avoid \"write_in_full(fd, buf, len) != len\" pattern\n> [4/7]: convert less-trivial versions of \"write_in_full() != len\"\n> [5/7]: pkt-line: check write_in_full() errors against \"< 0\"\n> [6/7]: notes-merge: use ssize_t for write_in_full() return value\n> [7/7]: config: flip return value of store_write_*()\n\nJonathan pointed out that read_in_full() can suffer from the same issue,\nand there is one buggy caller.\n\nA mass cleanup is a bit harder, as described in\n\n https://public-inbox.org/git/20170913183700.amtrquhg66hjrbsp@sigill.intra.peff.net/\n\nso I punted on it for now.\n\n-- >8 --\nSubject: [PATCH] read_pack_header: handle signed/unsigned comparison in read\n result\n\nThe result of read_in_full() may be -1 if we saw an error.\nBut in comparing it to a sizeof() result, that \"-1\" will be\npromoted to size_t. In fact, the largest possible size_t\nwhich is much bigger than our struct size. This means that\nour \"< sizeof(header)\" error check won't trigger.\n\nIn practice, we'd go on to read uninitialized memory and\ncompare it to the PACK signature, which is likely to fail.\nBut we shouldn't get there.\n\nWe can fix this by making a direct \"!=\" comparison to the\nrequested size, rather than \"<\". This means that errors get\nlumped in with short reads, but that's sufficient for our\npurposes here. There's no PH_ERROR tp represent our case.\nAnd anyway, this function reads from pipes and network\nsockets. A network error may racily appear as EOF to us\nanyway if there's data left in the socket buffers.\n\nSigned-off-by: Jeff King \n---\n sha1_file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5f71bbac3e..c1c6e9021d 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1850,7 +1850,7 @@ int index_path(struct object_id *oid, const char *path, struct stat *st, unsigne\n \n int read_pack_header(int fd, struct pack_header *header)\n {\n-\tif (read_in_full(fd, header, sizeof(*header)) < sizeof(*header))\n+\tif (read_in_full(fd, header, sizeof(*header)) != sizeof(*header))\n \t\t/* \"eof before pack header was fully read\" */\n \t\treturn PH_ERROR_EOF;\n \n-- \n2.14.1.874.ge7b2e05270\n\n"},{"id":"327987","messageId":"20170913185848.5ht44spbakzkrsu6@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913182431.GE27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH v2] config: avoid \"write_in_full(fd, buf, len) < len\" pattern","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T18:58:48Z","receivedAt":"2017-09-13T18:58:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2017 at 11:24:31AM -0700, Jonathan Nieder wrote:\n\n> For what it's worth,\n> Reviewed-by: Jonathan Nieder \n\nThanks, and thank you for questioning the ptrdiff_t bits that didn't\nmake sense. I _thought_ they felt like nonsense, but couldn't quite\npuzzle it out.\n\n> Compilers' signed/unsigned comparison warning can be noisy, but I'm\n> starting to feel it's worth the suppression noise to turn it on when\n> DEVELOPER=1 anyway. What do you think? Is there a way to turn it on\n> selectively for certain functions on the LHS (like read() and write()\n> style functions)?\n\nObviously we couldn't turn them on for -Werror at this point. Let me see\nhow bad \"-Wsign-compare -Wno-error=sign-compare\" is.\n\n $ make 2>&1 | grep -c warning:\n 1391\n\nOuch. I'm afraid that's probably not going to be very helpful. Even if I\nwere introducing a new problem, I'm not likely to see it amidst the mass\nof existing complaints.\n\nAFAIK there's no way to turn it on for specific functions, but I'm far\nfrom a gcc-warning guru. Even if you could, though, the error may be far\nfrom the function (e.g., if we store the result in an ssize_t and then\ncompare that with a size_t).\n\nInterestingly in the write_in_full() case the code was actually\nunreachable! But I don't think the compiler is smart enough to know\nthat, because it would have to realize that write_in_full() can only\nreturn either -1 or the original value (and not a positive value in\nbetween). Coverity _might_ be smart enough, but figuring that out does\ntake some loop analysis (including the assumption that write() never\nreturns more bytes than we asked it to write).\n\n-Peff\n"},{"id":"327989","messageId":"20170913191102.GF27425@aiede.mtv.corp.google.com","threadId":"46742","inReplyTo":"20170913184722.imcexldnufskugqz@sigill.intra.peff.net","subject":"Re: [PATCH 8/7] read_pack_header: handle signed/unsigned comparison in read result","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-13T19:11:02Z","receivedAt":"2017-09-13T19:11:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Subject: [PATCH] read_pack_header: handle signed/unsigned comparison in read result\n>\n> The result of read_in_full() may be -1 if we saw an error.\n> But in comparing it to a sizeof() result, that \"-1\" will be\n> promoted to size_t. In fact, the largest possible size_t\n> which is much bigger than our struct size. This means that\n> our \"< sizeof(header)\" error check won't trigger.\n>\n> In practice, we'd go on to read uninitialized memory and\n> compare it to the PACK signature, which is likely to fail.\n> But we shouldn't get there.\n>\n> We can fix this by making a direct \"!=\" comparison to the\n> requested size, rather than \"<\". This means that errors get\n> lumped in with short reads, but that's sufficient for our\n> purposes here. There's no PH_ERROR tp represent our case.\n> And anyway, this function reads from pipes and network\n> sockets. A network error may racily appear as EOF to us\n> anyway if there's data left in the socket buffers.\n>\n> Signed-off-by: Jeff King \n> ---\n> sha1_file.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nThis patch 8 (but not patches 2-7) is\nReviewed-by: Jonathan Nieder \n\nThanks.\n\nAn alternative would be to use something like\n\n\t< (int) sizeof(*header)\n\nto force it to be signed, but I think this is clearer.\n\nUsing the following semantic patch, I find this and the example from\npatch 1 and no others:\n\n @@\n expression fd;\n expression buf;\n expression len;\n size_t rhs;\n @@\n -read_in_full(fd, buf, len) < rhs\n +ERROR()\n\n @@\n expression fd;\n expression buf;\n expression len;\n size_t rhs;\n @@\n -write_in_full(fd, buf, len) < rhs\n +ERROR()\n\nThanks,\nJonathan\n\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1850,7 +1850,7 @@ int index_path(struct object_id *oid, const char *path, struct stat *st, unsigne\n> \n> int read_pack_header(int fd, struct pack_header *header)\n> {\n> -\tif (read_in_full(fd, header, sizeof(*header)) < sizeof(*header))\n> +\tif (read_in_full(fd, header, sizeof(*header)) != sizeof(*header))\n> \t\t/* \"eof before pack header was fully read\" */\n> \t\treturn PH_ERROR_EOF;\n> \n"},{"id":"327990","messageId":"20170913191840.GG27425@aiede.mtv.corp.google.com","threadId":"46742","inReplyTo":"20170913185848.5ht44spbakzkrsu6@sigill.intra.peff.net","subject":"Re: [PATCH v2] config: avoid \"write_in_full(fd, buf, len) < len\" pattern","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-13T19:18:40Z","receivedAt":"2017-09-13T19:18:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Wed, Sep 13, 2017 at 11:24:31AM -0700, Jonathan Nieder wrote:\n\n>> Compilers' signed/unsigned comparison warning can be noisy, but I'm\n>> starting to feel it's worth the suppression noise to turn it on when\n>> DEVELOPER=1 anyway. What do you think? Is there a way to turn it on\n>> selectively for certain functions on the LHS (like read() and write()\n>> style functions)?\n>\n> Obviously we couldn't turn them on for -Werror at this point. Let me see\n> how bad \"-Wsign-compare -Wno-error=sign-compare\" is.\n>\n> $ make 2>&1 | grep -c warning:\n> 1391\n>\n> Ouch. I'm afraid that's probably not going to be very helpful. Even if I\n> were introducing a new problem, I'm not likely to see it amidst the mass\n> of existing complaints.\n\nYup.\n\nSomething like http://coccinelle.lip6.fr/rules/find_unsigned.html (but\nnot that one precisely, since it's not smart enough to follow typedefs\nlike size_t!) can do this for specific functions. Then \"make\ncoccicheck\" would report new problematic usages. Let me know if this\nseems worth pursuing, and I'll send a patch based on the spatch I used\nto review your patch 8.\n\nUsing coccinelle for this is a kind of oversized hammer. I wonder if\ne.g. sparse would be able to do a better job.\n\n> AFAIK there's no way to turn it on for specific functions, but I'm far\n> from a gcc-warning guru. Even if you could, though, the error may be far\n> from the function (e.g., if we store the result in an ssize_t and then\n> compare that with a size_t).\n\nIf we have a check that catches the obvious cases then I'm already\npretty happy.\n\nThough checking ssize_t vs size_t comparisons in general might also\nnot be a bad idea, since it would be narrower than the general\n-Wsign-compare check.\n\nThanks,\nJonathan\n"},{"id":"327992","messageId":"20170913194917.GI27425@aiede.mtv.corp.google.com","threadId":"46742","inReplyTo":"20170913185848.5ht44spbakzkrsu6@sigill.intra.peff.net","subject":"Re: [PATCH v2] config: avoid \"write_in_full(fd, buf, len) < len\" pattern","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-13T19:49:17Z","receivedAt":"2017-09-13T19:49:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> AFAIK there's no way to turn it on for specific functions, but I'm far\n> from a gcc-warning guru. Even if you could, though, the error may be far\n> from the function (e.g., if we store the result in an ssize_t and then\n> compare that with a size_t).\n\nIt turns out that yes, we have two of those. Both handle errors\nseparately already, so they should be safe.\n\nWhile investigating the second, I noticed that read_in_full can\noverflow its return value. malloc doesn't typically allow allocating\na buffer with size greater than SSIZE_MAX so this should be safe, but\nit would be confusing to static analyzers.\n\nCombining these observations yields the following (just for\nillustration):\n\ndiff --git i/bulk-checkin.c w/bulk-checkin.c\nindex 9a1f6c49ab..f8e3060041 100644\n--- i/bulk-checkin.c\n+++ w/bulk-checkin.c\n@@ -114,7 +114,7 @@ static int stream_to_pack(struct bulk_checkin_state *state,\n \t\tunsigned char ibuf[16384];\n \n \t\tif (size && !s.avail_in) {\n-\t\t\tssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);\n+\t\t\tsize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);\n \t\t\tif (read_in_full(fd, ibuf, rsize) != rsize)\n \t\t\t\tdie(\"failed to read %d bytes from '%s'\",\n \t\t\t\t (int)rsize, path);\ndiff --git i/combine-diff.c w/combine-diff.c\nindex 9e163d5ada..e966d4f393 100644\n--- i/combine-diff.c\n+++ w/combine-diff.c\n@@ -1026,7 +1026,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tresult_size = fill_textconv(textconv, df, &result);\n \t\t\tfree_filespec(df);\n \t\t} else if (0 <= (fd = open(elem->path, O_RDONLY))) {\n-\t\t\tsize_t len = xsize_t(st.st_size);\n+\t\t\tssize_t len = xssize_t(st.st_size);\n \t\t\tssize_t done;\n \t\t\tint is_file, i;\n \n@@ -1040,6 +1040,8 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tif (!is_file)\n \t\t\t\telem->mode = canon_mode(S_IFLNK);\n \n+\t\t\tif (len > ULONG_MAX)\n+\t\t\t\tdie(\"cannot handle files this big\");\n \t\t\tresult_size = len;\n \t\t\tresult = xmallocz(len);\n \ndiff --git i/git-compat-util.h w/git-compat-util.h\nindex 6678b488cc..20fea81589 100644\n--- i/git-compat-util.h\n+++ w/git-compat-util.h\n@@ -903,6 +903,13 @@ static inline size_t xsize_t(off_t len)\n \treturn (size_t)len;\n }\n \n+static inline ssize_t xssize_t(off_t len)\n+{\n+\tif (len > SSIZE_MAX)\n+\t\tdie(\"cannot handle files this big\");\n+\treturn (ssize_t)len;\n+}\n+\n __attribute__((format (printf, 3, 4)))\n extern int xsnprintf(char *dst, size_t max, const char *fmt, ...);\n \ndiff --git i/wrapper.c w/wrapper.c\nindex 36630e5d18..2b52b7367d 100644\n--- i/wrapper.c\n+++ w/wrapper.c\n@@ -314,6 +314,9 @@ ssize_t read_in_full(int fd, void *buf, size_t count)\n \tchar *p = buf;\n \tssize_t total = 0;\n \n+\tif (count > SSIZE_MAX)\n+\t\tBUG(\"read_in_full called with absurdly high count %\"PRIuMAX,\n+\t\t (uintmax_t) count);\n \twhile (count > 0) {\n \t\tssize_t loaded = xread(fd, p, count);\n \t\tif (loaded < 0)\n"},{"id":"327993","messageId":"20170913210927.GJ27425@aiede.mtv.corp.google.com","threadId":"46742","inReplyTo":"20170913171127.ocnitq6wz57ivhrh@sigill.intra.peff.net","subject":"Re: [PATCH 2/7] get-tar-commit-id: check write_in_full() return against 0","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-13T21:09:27Z","receivedAt":"2017-09-13T21:09:35Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> We ask to write 41 bytes and make sure that the return value\n> is at least 41. This is the same \"dangerous\" pattern that\n> was fixed in the prior commit (wherein a negative return\n> value is promoted to unsigned), though it is not dangerous\n> here because our \"41\" is a constant, not an unsigned\n> variable.\n>\n> But we should convert it anyway to avoid modeling a\n> dangerous construct.\n>\n> Signed-off-by: Jeff King \n> ---\n> builtin/get-tar-commit-id.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n\nI kind of disagree with calling this dangerous (and I think that is\nwhat you alluded to above by putting it in quotes), but I like the\npostimage more than the preimage.\n\nThe variable 'n' could be eliminated to simplify this further. I\nrealize that would go against the spirit of this patch, but (1) it's\non-topic for the patch, since it is another ssize_t vs constant\ncomparison and (2) as mentioned elsewhere in this thread, it's a very\ncommon idiom with read_in_full. If we want to eliminate it then we\ncould introduce a separate helper to distinguish between\nread_this_much_i_mean_it and read_this_much_or_to_eof.\n\nAnyway, I think the below is an improvement.\n\nWith or without this tweak,\nReviewed-by: Jonathan Nieder \n\nThanks.\n\ndiff --git i/builtin/get-tar-commit-id.c w/builtin/get-tar-commit-id.c\nindex 6d9a79f9b3..03ef7c5ba4 100644\n--- i/builtin/get-tar-commit-id.c\n+++ w/builtin/get-tar-commit-id.c\n@@ -20,13 +20,11 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix)\n \tstruct ustar_header *header = (struct ustar_header *)buffer;\n \tchar *content = buffer + RECORDSIZE;\n \tconst char *comment;\n-\tssize_t n;\n \n \tif (argc != 1)\n \t\tusage(builtin_get_tar_commit_id_usage);\n \n-\tn = read_in_full(0, buffer, HEADERSIZE);\n-\tif (n < HEADERSIZE)\n+\tif (read_in_full(0, buffer, HEADERSIZE) < HEADERSIZE)\n \t\tdie(\"git get-tar-commit-id: read error\");\n \tif (header->typeflag[0] != 'g')\n \t\treturn 1;\n"},{"id":"327994","messageId":"20170913211430.GK27425@aiede.mtv.corp.google.com","threadId":"46742","inReplyTo":"20170913171603.3whuql7l5da3ynrn@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] avoid \"write_in_full(fd, buf, len) != len\" pattern","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-13T21:14:30Z","receivedAt":"2017-09-13T21:14:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> The return value of write_in_full() is either \"-1\", or the\n> requested number of bytes[1]. If we make a partial write\n> before seeing an error, we still return -1, not a partial\n> value. This goes back to f6aa66cb95 (write_in_full: really\n> write in full or return error on disk full., 2007-01-11).\n>\n> So checking anything except \"was the return value negative\"\n> is pointless. And there are a couple of reasons not to do\n> so:\n>\n> 1. It can do a funny signed/unsigned comparison. If your\n[...]\n> 2. Checking for a negative value is shorter to type,\n> especially when the length is an expression.\n>\n> 3. Linus says so. In d34cf19b89 (Clean up write_in_full()\n> users, 2007-01-11), right after the write_in_full()\n> semantics were changed, he wrote:\n>\n> I really wish every \"write_in_full()\" user would just\n> check against \"<0\" now, but this fixes the nasty and\n> stupid ones.\n\nOk, you convinced me.\n\nShould we add a comment to cache.h as well encouraging this?\n\n[...]\n> [1] A careful reader may notice there is one way that\n> write_in_full() can return a different value. If we ask\n> write() to write N bytes and get a return value that is\n> _larger_ than N, we could return a larger total. But\n> besides the fact that this would imply a totally broken\n> version of write(), it would already invoke undefined\n> behavior. Our internal remaining counter is an unsigned\n> size_t, which means that subtracting too many byte will\n> wrap it around to a very large number. So we'll instantly\n> begin reading off the end of the buffer, trying to write\n> gigabytes (or petabytes) of data.\n\nThis footnote just leaves me more confused, since as you mention,\nwrite() never would return a value greater than N. Are you saying we\nneed to defend against a broken platform where that isn't true?\n\n> Signed-off-by: Jeff King \n> ---\n> builtin/receive-pack.c | 2 +-\n> builtin/rerere.c | 2 +-\n> builtin/unpack-file.c | 2 +-\n> config.c | 4 ++--\n> diff.c | 2 +-\n> fast-import.c | 2 +-\n> http-backend.c | 4 ++--\n> ll-merge.c | 2 +-\n> read-cache.c | 6 +++---\n> refs.c | 2 +-\n> refs/files-backend.c | 8 ++++----\n> rerere.c | 2 +-\n> shallow.c | 6 +++---\n> t/helper/test-delta.c | 2 +-\n> transport-helper.c | 5 ++---\n> wrapper.c | 2 +-\n> 16 files changed, 26 insertions(+), 27 deletions(-)\n\nAll of these look correctly done.\n\nReviewed-by: Jonathan Nieder \n"},{"id":"327995","messageId":"20170913211616.GL27425@aiede.mtv.corp.google.com","threadId":"46742","inReplyTo":"20170913171628.o4euxlzk4imlf4tv@sigill.intra.peff.net","subject":"Re: [PATCH 4/7] convert less-trivial versions of \"write_in_full() != len\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-13T21:16:16Z","receivedAt":"2017-09-13T21:16:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> The prior commit converted many sites to check the return\n> value of write_in_full() for negativity, rather than a\n> mismatch with the input length. This patch covers similar\n> cases, but where the return value is stored in an\n> intermediate variable. These should get the same treatment,\n> but they need to be reviewed more carefully since it would\n> be a bug if the return value is stored in an unsigned type\n> (which indeed, it is in one of the cases).\n>\n> Signed-off-by: Jeff King \n> ---\n> entry.c | 5 +++--\n> refs/files-backend.c | 2 +-\n> streaming.c | 2 +-\n> 3 files changed, 5 insertions(+), 4 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"327996","messageId":"20170913211742.GM27425@aiede.mtv.corp.google.com","threadId":"46742","inReplyTo":"20170913171730.ckcebchjhhvtwcsj@sigill.intra.peff.net","subject":"Re: [PATCH 5/7] pkt-line: check write_in_full() errors against \"< 0\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-13T21:17:42Z","receivedAt":"2017-09-13T21:17:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> As with the previous two commits, we prefer to check\n> write_in_full()'s return value to see if it is negative,\n> rather than comparing it to the input length.\n>\n> These cases actually flip the logic to check for success,\n> making conversion a little different than in other cases. We\n> could of course write:\n>\n> if (write_in_full(...) >= 0)\n> return 0;\n> return error(...);\n>\n> But our usual method of spelling write() error checks is\n> just \"< 0\". So let's flip the logic for each of these\n> conditionals to our usual style.\n>\n> Signed-off-by: Jeff King \n> ---\n> pkt-line.c | 29 ++++++++++++++---------------\n> 1 file changed, 14 insertions(+), 15 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"327997","messageId":"20170913212035.GN27425@aiede.mtv.corp.google.com","threadId":"46742","inReplyTo":"20170913171744.32cmtjkdcp7yf7pt@sigill.intra.peff.net","subject":"Re: [PATCH 6/7] notes-merge: use ssize_t for write_in_full() return value","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-13T21:20:35Z","receivedAt":"2017-09-13T21:20:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> We store the return value of write_in_full() in a long,\n> though the return is actually an ssize_t. This probably\n> doesn't matter much in practice (since the buffer size is\n> alredy an unsigned long), but it might if the size if\n> between what can be represented in \"long\" and \"unsigned\n> long\", and if your size_t is larger than a \"long\" (as it is\n> on 64-bit Windows).\n>\n> Signed-off-by: Jeff King \n> ---\n> notes-merge.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nYikes. Good catch.\n\nWith or without the tweak below,\nReviewed-by: Jonathan Nieder \n\n> --- a/notes-merge.c\n> +++ b/notes-merge.c\n> @@ -302,7 +302,7 @@ static void write_buf_to_worktree(const struct object_id *obj,\n> \tfd = xopen(path, O_WRONLY | O_EXCL | O_CREAT, 0666);\n> \n> \twhile (size > 0) {\n> -\t\tlong ret = write_in_full(fd, buf, size);\n> +\t\tssize_t ret = write_in_full(fd, buf, size);\n> \t\tif (ret < 0) {\n> \t\t\t/* Ignore epipe */\n> \t\t\tif (errno == EPIPE)\n> \t\t\t\tbreak;\n> \t\t\tdie_errno(\"notes-merge\");\n> \t\t} else if (!ret) {\n> \t\t\tdie(\"notes-merge: disk full?\");\n> \t\t}\n\nThese three lines are dead code. How about the following, e.g. for\nsquashing in?\n\ndiff --git i/notes-merge.c w/notes-merge.c\nindex 597d43f65c..4352c34a6e 100644\n--- i/notes-merge.c\n+++ w/notes-merge.c\n@@ -308,8 +308,6 @@ static void write_buf_to_worktree(const struct object_id *obj,\n \t\t\tif (errno == EPIPE)\n \t\t\t\tbreak;\n \t\t\tdie_errno(\"notes-merge\");\n-\t\t} else if (!ret) {\n-\t\t\tdie(\"notes-merge: disk full?\");\n \t\t}\n \t\tsize -= ret;\n \t\tbuf += ret;\n"},{"id":"327998","messageId":"20170913212528.GO27425@aiede.mtv.corp.google.com","threadId":"46742","inReplyTo":"20170913171756.v7vzu3a77g2khq7x@sigill.intra.peff.net","subject":"Re: [PATCH 7/7] config: flip return value of store_write_*()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-13T21:25:28Z","receivedAt":"2017-09-13T21:25:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> The store_write_section() and store_write_pairs() functions\n> are basically high-level wrappers around write(). But their\n> return values are flipped from our usual convention, using\n> \"1\" for success and \"0\" for failure.\n>\n> Let's flip them to follow the usual write() conventions and\n> update all callers. As these are local to config.c, it's\n> unlikely that we'd have new callers in any topics in flight\n> (which would be silently broken by our change). But just to\n> be on the safe side, let's rename them to just\n> write_section() and write_pairs(). That also accentuates\n> their relationship with write().\n>\n> Signed-off-by: Jeff King \n\nThe caller only cares if it succeeded, right? Could this return\nthe customary 0 vs -1 instead of the number of bytes written?\n\nThat would look like the following (as a patch to squash in):\n\nWith or without that tweak,\nReviewed-by: Jonathan Nieder \n\ndiff --git i/config.c w/config.c\nindex 272a32aac0..8f92d452bf 100644\n--- i/config.c\n+++ w/config.c\n@@ -2297,11 +2297,10 @@ static int write_error(const char *filename)\n \treturn 4;\n }\n \n-static ssize_t write_section(int fd, const char *key)\n+static int write_section(int fd, const char *key)\n {\n \tconst char *dot;\n-\tint i;\n-\tssize_t ret;\n+\tint i, ret;\n \tstruct strbuf sb = STRBUF_INIT;\n \n \tdot = memchr(key, '.', store.baselen);\n@@ -2317,16 +2316,15 @@ static ssize_t write_section(int fd, const char *key)\n \t\tstrbuf_addf(&sb, \"[%.*s]\\n\", store.baselen, key);\n \t}\n \n-\tret = write_in_full(fd, sb.buf, sb.len);\n+\tret = write_in_full(fd, sb.buf, sb.len) < 0 ? -1 : 0;\n \tstrbuf_release(&sb);\n \n \treturn ret;\n }\n \n-static ssize_t write_pair(int fd, const char *key, const char *value)\n+static int write_pair(int fd, const char *key, const char *value)\n {\n-\tint i;\n-\tssize_t ret;\n+\tint i, ret;\n \tint length = strlen(key + store.baselen + 1);\n \tconst char *quote = \"\";\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -2366,7 +2364,7 @@ static ssize_t write_pair(int fd, const char *key, const char *value)\n \t\t}\n \tstrbuf_addf(&sb, \"%s\\n\", quote);\n \n-\tret = write_in_full(fd, sb.buf, sb.len);\n+\tret = write_in_full(fd, sb.buf, sb.len) < 0 ? -1 : 0;\n \tstrbuf_release(&sb);\n \n \treturn ret;\n@@ -2499,8 +2497,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t}\n \n \t\tstore.key = (char *)key;\n-\t\tif (write_section(fd, key) < 0 ||\n-\t\t write_pair(fd, key, value) < 0)\n+\t\tif (write_section(fd, key) || write_pair(fd, key, value))\n \t\t\tgoto write_err_out;\n \t} else {\n \t\tstruct stat st;\n@@ -2623,10 +2620,10 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t/* write the pair (value == NULL means unset) */\n \t\tif (value != NULL) {\n \t\t\tif (store.state == START) {\n-\t\t\t\tif (write_section(fd, key) < 0)\n+\t\t\t\tif (write_section(fd, key))\n \t\t\t\t\tgoto write_err_out;\n \t\t\t}\n-\t\t\tif (write_pair(fd, key, value) < 0)\n+\t\t\tif (write_pair(fd, key, value))\n \t\t\t\tgoto write_err_out;\n \t\t}\n \n@@ -2820,7 +2817,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n \t\t\t\tstore.baselen = strlen(new_name);\n-\t\t\t\tif (write_section(out_fd, new_name) < 0) {\n+\t\t\t\tif (write_section(out_fd, new_name)) {\n \t\t\t\t\tret = write_error(get_lock_file_path(lock));\n \t\t\t\t\tgoto out;\n \t\t\t\t}\n"},{"id":"327999","messageId":"xmqqfubq1dy4.fsf@gitster.mtv.corp.google.com","threadId":"46742","inReplyTo":"20170913182431.GE27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH v2] config: avoid \"write_in_full(fd, buf, len) < len\" pattern","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-13T21:33:23Z","receivedAt":"2017-09-13T21:33:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Jeff King wrote:\n>\n>> What I missed is that copy_begin and copy_end here are actually size_t\n>> variables, not the pointers. Sorry for the confusion, and here's an\n>> updated version of the patch with this paragraph amended (the patch\n>> itself is identical):\n>\n> Subtle. The world makes more sense now. Thanks for figuring it out.\n> ...\n> For what it's worth,\n> Reviewed-by: Jonathan Nieder \n\nThanks, both. It seems that I missed all the fun ;-)\n"},{"id":"328018","messageId":"ef20628e-b7c0-8909-31a0-cc126d0c40ba@ramsayjones.plus.com","threadId":"46742","inReplyTo":"20170913185848.5ht44spbakzkrsu6@sigill.intra.peff.net","subject":"Re: [PATCH v2] config: avoid \"write_in_full(fd, buf, len) < len\" pattern","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-09-13T22:43:51Z","receivedAt":"2017-09-13T22:44:00Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 13/09/17 19:58, Jeff King wrote:\n> On Wed, Sep 13, 2017 at 11:24:31AM -0700, Jonathan Nieder wrote:\n> \n>> For what it's worth,\n>> Reviewed-by: Jonathan Nieder \n> \n> Thanks, and thank you for questioning the ptrdiff_t bits that didn't\n> make sense. I _thought_ they felt like nonsense, but couldn't quite\n> puzzle it out.\n> \n>> Compilers' signed/unsigned comparison warning can be noisy, but I'm\n>> starting to feel it's worth the suppression noise to turn it on when\n>> DEVELOPER=1 anyway. What do you think? Is there a way to turn it on\n>> selectively for certain functions on the LHS (like read() and write()\n>> style functions)?\n> \n> Obviously we couldn't turn them on for -Werror at this point. Let me see\n> how bad \"-Wsign-compare -Wno-error=sign-compare\" is.\n> \n> $ make 2>&1 | grep -c warning:\n> 1391\n> \n> Ouch. I'm afraid that's probably not going to be very helpful. Even if I\n> were introducing a new problem, I'm not likely to see it amidst the mass\n> of existing complaints.\n\nHmm, about three or four years ago, I spent two or three evenings\ngetting git to compile -Wextra clean. I remember the signed/unsigned\nissue was the cause of a large portion of the warnings issued by\nthe compiler. I was surprised that it took such a relatively short\ntime to do. However, it affected quite a large portion of the code, so\nI didn't think Junio would even consider applying it. Also, I only used\ngcc and was anticipating having additional warnings on clang (but I\ndidn't get around to trying).\n\nMaybe I should give it another go. :-D\n\nATB,\nRamsay Jones\n\n"},{"id":"328022","messageId":"1012b31b-7bdd-ea20-f004-1c9f80de733c@ramsayjones.plus.com","threadId":"46742","inReplyTo":"ef20628e-b7c0-8909-31a0-cc126d0c40ba@ramsayjones.plus.com","subject":"Re: [PATCH v2] config: avoid \"write_in_full(fd, buf, len) < len\" pattern","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-09-13T23:31:38Z","receivedAt":"2017-09-13T23:31:47Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 13/09/17 23:43, Ramsay Jones wrote:\n> \n> \n> On 13/09/17 19:58, Jeff King wrote:\n>> On Wed, Sep 13, 2017 at 11:24:31AM -0700, Jonathan Nieder wrote:\n>>\n>>> For what it's worth,\n>>> Reviewed-by: Jonathan Nieder \n>>\n>> Thanks, and thank you for questioning the ptrdiff_t bits that didn't\n>> make sense. I _thought_ they felt like nonsense, but couldn't quite\n>> puzzle it out.\n>>\n>>> Compilers' signed/unsigned comparison warning can be noisy, but I'm\n>>> starting to feel it's worth the suppression noise to turn it on when\n>>> DEVELOPER=1 anyway. What do you think? Is there a way to turn it on\n>>> selectively for certain functions on the LHS (like read() and write()\n>>> style functions)?\n>>\n>> Obviously we couldn't turn them on for -Werror at this point. Let me see\n>> how bad \"-Wsign-compare -Wno-error=sign-compare\" is.\n>>\n>> $ make 2>&1 | grep -c warning:\n>> 1391\n>>\n>> Ouch. I'm afraid that's probably not going to be very helpful. Even if I\n>> were introducing a new problem, I'm not likely to see it amidst the mass\n>> of existing complaints.\n> \n> Hmm, about three or four years ago, I spent two or three evenings\n> getting git to compile -Wextra clean. I remember the signed/unsigned\n> issue was the cause of a large portion of the warnings issued by\n> the compiler. I was surprised that it took such a relatively short\n> time to do. However, it affected quite a large portion of the code, so\n> I didn't think Junio would even consider applying it. Also, I only used\n> gcc and was anticipating having additional warnings on clang (but I\n> didn't get around to trying).\n> \n> Maybe I should give it another go. :-D\n\nFor example, I remember the patch given below reduced the number\nof warnings quite a bit (because it's an inline function in a\nheader file).\n\nI just tried it again tonight; the current master branch has 3532\nwarnings when compiled with -Wextra, 1409 of which are -Wsign-compare\nwarnings. After applying the patch below, those numbers are reduced\nby 344 to 3188/1065.\n\nStill quite a bit to go ... ;-)\n\nATB,\nRamsay Jones\n\n-- >8 --\nSubject: [PATCH] git-compat-util: avoid -Wsign-compare warnings from xsize_t()\n\nSigned-off-by: Ramsay Jones \n---\n git-compat-util.h | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 6678b488c..2f3cf0883 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -898,9 +898,11 @@ static inline char *xstrdup_or_null(const char *str)\n \n static inline size_t xsize_t(off_t len)\n {\n-\tif (len > (size_t) len)\n+\tsize_t size = (size_t) len;\n+\n+\tif (len != (off_t) size)\n \t\tdie(\"Cannot handle files this big\");\n-\treturn (size_t)len;\n+\treturn size;\n }\n \n __attribute__((format (printf, 3, 4)))\n-- \n2.14.0\n"},{"id":"328085","messageId":"20170915003754.wlckhuwcftyc32de@sigill.intra.peff.net","threadId":"46742","inReplyTo":"1012b31b-7bdd-ea20-f004-1c9f80de733c@ramsayjones.plus.com","subject":"Re: [PATCH v2] config: avoid \"write_in_full(fd, buf, len) < len\" pattern","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-15T00:37:54Z","receivedAt":"2017-09-15T00:38:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 14, 2017 at 12:31:38AM +0100, Ramsay Jones wrote:\n\n> > Hmm, about three or four years ago, I spent two or three evenings\n> > getting git to compile -Wextra clean. I remember the signed/unsigned\n> > issue was the cause of a large portion of the warnings issued by\n> > the compiler. I was surprised that it took such a relatively short\n> > time to do. However, it affected quite a large portion of the code, so\n> > I didn't think Junio would even consider applying it. Also, I only used\n> > gcc and was anticipating having additional warnings on clang (but I\n> > didn't get around to trying).\n> > \n> > Maybe I should give it another go. :-D\n> \n> For example, I remember the patch given below reduced the number\n> of warnings quite a bit (because it's an inline function in a\n> header file).\n> \n> I just tried it again tonight; the current master branch has 3532\n> warnings when compiled with -Wextra, 1409 of which are -Wsign-compare\n> warnings. After applying the patch below, those numbers are reduced\n> by 344 to 3188/1065.\n\nI'd love it if we were clean on -Wextra. My big question is many\ncontortions we'd have to go through in the code. I don't mind at all if\nwe're actually cleaning as we go (e.g., using types of the correct\nsignedness, or preventing possible funny interactions). I'm just worried\nit will turn into a bunch of noisy casts.\n\nThe patch you showed seems like an improvement to the code, but I don't\nknow if that would be the case for all of them. :)\n\n-Peff\n"},{"id":"328086","messageId":"20170915004039.sxluh3cv7p6tltqa@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913210927.GJ27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH 2/7] get-tar-commit-id: check write_in_full() return against 0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-15T00:40:39Z","receivedAt":"2017-09-15T00:40:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2017 at 02:09:27PM -0700, Jonathan Nieder wrote:\n\n> > We ask to write 41 bytes and make sure that the return value\n> > is at least 41. This is the same \"dangerous\" pattern that\n> > was fixed in the prior commit (wherein a negative return\n> > value is promoted to unsigned), though it is not dangerous\n> > here because our \"41\" is a constant, not an unsigned\n> > variable.\n> >\n> > But we should convert it anyway to avoid modeling a\n> > dangerous construct.\n> >\n> > Signed-off-by: Jeff King \n> > ---\n> > builtin/get-tar-commit-id.c | 3 +--\n> > 1 file changed, 1 insertion(+), 2 deletions(-)\n> \n> I kind of disagree with calling this dangerous (and I think that is\n> what you alluded to above by putting it in quotes), but I like the\n> postimage more than the preimage.\n\nRight, this instance is fine, but the pattern of using \"<\" is not. If\nyou swapped out \"41\" for:\n\n size_t len = 41;\n\nthen it would be a bug. Which I think would surprise most people.\n\n> The variable 'n' could be eliminated to simplify this further. I\n> realize that would go against the spirit of this patch, but (1) it's\n> on-topic for the patch, since it is another ssize_t vs constant\n> comparison and (2) as mentioned elsewhere in this thread, it's a very\n> common idiom with read_in_full. If we want to eliminate it then we\n> could introduce a separate helper to distinguish between\n> read_this_much_i_mean_it and read_this_much_or_to_eof.\n\nYes, I noticed that, too, after you brought up read_in_full() as a\npotential source of problems. But I would rather deal with\nread_in_full() separately on top. Can you do it as a separate patch on\ntop (possibly with other read_in_full() cleanups, though I think this is\nthe only \"<\" case that exists).\n\n-Peff\n"},{"id":"328087","messageId":"20170915004237.wjys6tgyxk6j43op@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913211430.GK27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH 3/7] avoid \"write_in_full(fd, buf, len) != len\" pattern","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-15T00:42:37Z","receivedAt":"2017-09-15T00:42:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2017 at 02:14:30PM -0700, Jonathan Nieder wrote:\n\n> > I really wish every \"write_in_full()\" user would just\n> > check against \"<0\" now, but this fixes the nasty and\n> > stupid ones.\n> \n> Ok, you convinced me.\n> \n> Should we add a comment to cache.h as well encouraging this?\n\nI'd be OK with a comment, though I don't know that it's strictly\nnecessary. It looks like most of it was just cargo-culted, so removing\nthe offending examples is sufficient.\n\n> > [1] A careful reader may notice there is one way that\n> > write_in_full() can return a different value. If we ask\n> > write() to write N bytes and get a return value that is\n> > _larger_ than N, we could return a larger total. But\n> > besides the fact that this would imply a totally broken\n> > version of write(), it would already invoke undefined\n> > behavior. Our internal remaining counter is an unsigned\n> > size_t, which means that subtracting too many byte will\n> > wrap it around to a very large number. So we'll instantly\n> > begin reading off the end of the buffer, trying to write\n> > gigabytes (or petabytes) of data.\n> \n> This footnote just leaves me more confused, since as you mention,\n> write() never would return a value greater than N. Are you saying we\n> need to defend against a broken platform where that isn't true?\n\nNo, I'm saying that my claim that write_in_full() can only return two\nvalues (-1 and the original length) is not strictly true. But that it\ndoesn't matter in practice.\n\nI don't think we need to defend against such a broken platform, but I\ndidn't want anybody reading the claim to say \"aha, you forgot this\ncase\". It is a case that does not matter.\n\n-Peff\n"},{"id":"328088","messageId":"20170915004341.zwmpkvcvjgifn5zn@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913212035.GN27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH 6/7] notes-merge: use ssize_t for write_in_full() return value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-15T00:43:41Z","receivedAt":"2017-09-15T00:43:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2017 at 02:20:35PM -0700, Jonathan Nieder wrote:\n\n> > --- a/notes-merge.c\n> > +++ b/notes-merge.c\n> > @@ -302,7 +302,7 @@ static void write_buf_to_worktree(const struct object_id *obj,\n> > \tfd = xopen(path, O_WRONLY | O_EXCL | O_CREAT, 0666);\n> > \n> > \twhile (size > 0) {\n> > -\t\tlong ret = write_in_full(fd, buf, size);\n> > +\t\tssize_t ret = write_in_full(fd, buf, size);\n> > \t\tif (ret < 0) {\n> > \t\t\t/* Ignore epipe */\n> > \t\t\tif (errno == EPIPE)\n> > \t\t\t\tbreak;\n> > \t\t\tdie_errno(\"notes-merge\");\n> > \t\t} else if (!ret) {\n> > \t\t\tdie(\"notes-merge: disk full?\");\n> > \t\t}\n> \n> These three lines are dead code. How about the following, e.g. for\n> squashing in?\n\nThanks, I didn't notice that.\n\nI'd actually prefer it as a separate patch, since it needs explained\nseparately.\n\n-Peff\n"},{"id":"328089","messageId":"20170915004637.jueyykocyktrwfk7@sigill.intra.peff.net","threadId":"46742","inReplyTo":"20170913212528.GO27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH 7/7] config: flip return value of store_write_*()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-15T00:46:37Z","receivedAt":"2017-09-15T00:46:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2017 at 02:25:28PM -0700, Jonathan Nieder wrote:\n\n> > Let's flip them to follow the usual write() conventions and\n> > update all callers. As these are local to config.c, it's\n> > unlikely that we'd have new callers in any topics in flight\n> > (which would be silently broken by our change). But just to\n> > be on the safe side, let's rename them to just\n> > write_section() and write_pairs(). That also accentuates\n> > their relationship with write().\n> >\n> > Signed-off-by: Jeff King \n> \n> The caller only cares if it succeeded, right? Could this return\n> the customary 0 vs -1 instead of the number of bytes written?\n\nYes, it could. I went with \"follow the conventions of write()\" because\nthese are used in a big chain of write() calls (well, really\nwrite_in_full). But given the current callers, it does not matter either\nway.\n\nThanks for reviewing the series, and sorry if my comments have been a\nbit terse. I'm trying to clear my pile before going out of town for a\nfew days (which I admit may have contributed to my desire for you to\nprepare patches on top).\n\nBut either way, don't expect a re-roll until next week at the earliest.\n\n-Peff\n"},{"id":"328127","messageId":"0aa64f62-4acc-4235-d7c4-efb5986699fc@ramsayjones.plus.com","threadId":"46742","inReplyTo":"20170915003754.wlckhuwcftyc32de@sigill.intra.peff.net","subject":"Re: [PATCH v2] config: avoid \"write_in_full(fd, buf, len) < len\" pattern","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-09-15T15:15:14Z","receivedAt":"2017-09-15T15:15:22Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 15/09/17 01:37, Jeff King wrote:\n> On Thu, Sep 14, 2017 at 12:31:38AM +0100, Ramsay Jones wrote:\n> \n>> I just tried it again tonight; the current master branch has 3532\n>> warnings when compiled with -Wextra, 1409 of which are -Wsign-compare\n>> warnings. After applying the patch below, those numbers are reduced\n>> by 344 to 3188/1065.\n> \n> I'd love it if we were clean on -Wextra. My big question is many\n> contortions we'd have to go through in the code. I don't mind at all if\n> we're actually cleaning as we go (e.g., using types of the correct\n> signedness, or preventing possible funny interactions). I'm just worried\n> it will turn into a bunch of noisy casts.\n\nHmm, my memory is not what it was, ... ;-) However, I seem to recall\nthat most of the changes were \"improvements\", with only a (relatively)\nfew scattering of casts (behind existing macros - namely the OPTION\nmacros). (Oh, wait, I think 'unused' parameters etc., was another\nproblem area).\n\n> The patch you showed seems like an improvement to the code, but I don't\n> know if that would be the case for all of them. :)\n\nYes, I've had that patch (and others like it) hanging around for\nyears! (perhaps it's time to dig through all those old branches)\n\nATB,\nRamsay Jones\n\n\n"}]}