Bug: git branch --unset-upstream command can nuke config when disk is full.

After being away for a while I saw the following message in one of my git repos:

$ git status
On branch yves/xxx
Your branch is based on 'origin/yves/xxx', but the upstream is gone.
  (use "git branch --unset-upstream" to fixup)

nothing to commit, working tree clean
$ git branch --unset-upstream
fatal: could not unset 'branch.yves/simple_projection.merge'

At this point my .git/config file was empty, and all of my config was lost.

I assume that things that rewrite .git/config do not check for a
successful write before deleting the old version of the file.

This was git version 2.14.1

Yves



-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

On Wed, Sep 13, 2017 at 01:59:17PM +0200, demerphq wrote:

After being away for a while I saw the following message in one of my git repos:

$ git status
On branch yves/xxx
Your branch is based on 'origin/yves/xxx', but the upstream is gone.
  (use "git branch --unset-upstream" to fixup)

nothing to commit, working tree clean
$ git branch --unset-upstream
fatal: could not unset 'branch.yves/simple_projection.merge'

Hrm. I wonder what caused this failure. The error would be in
git_config_set_multivar_in_file_gently(). Most errors there produce
another error message before hitting the die(). In fact, the only case I
see where it would not produce another message is if it found nothing to
unset (but in that case, "branch" would never have called the function
in the first place).

At this point my .git/config file was empty, and all of my config was lost.

I assume that things that rewrite .git/config do not check for a
successful write before deleting the old version of the file.

No, it writes the new content to "config.lock" and then renames it into
place. All of the write() calls to the temporary file are checked. The
old data is copied over after having been ready by mmap (which is also
error-checked).

Given that your output is consistent with it failing to find the key,
and that the result is an empty file, it sounds like somehow the mmap'd
input appeared empty (but neither open nor fstat nor mmap returned an
error). You're not on any kind of exotic filesystem, are you?

-Peff

On 13 September 2017 at 14:34, Jeff King [off-list ref] wrote:

On Wed, Sep 13, 2017 at 01:59:17PM +0200, demerphq wrote:

quoted

After being away for a while I saw the following message in one of my git repos:

$ git status
On branch yves/xxx
Your branch is based on 'origin/yves/xxx', but the upstream is gone.
  (use "git branch --unset-upstream" to fixup)

nothing to commit, working tree clean
$ git branch --unset-upstream
fatal: could not unset 'branch.yves/simple_projection.merge'

Hrm. I wonder what caused this failure. The error would be in
git_config_set_multivar_in_file_gently(). Most errors there produce
another error message before hitting the die(). In fact, the only case I
see where it would not produce another message is if it found nothing to
unset (but in that case, "branch" would never have called the function
in the first place).

I just double checked the terminal history and this is all i saw:

$ git status
On branch yves/xxx
Your branch is based on 'origin/yves/xxx', but the upstream is gone.
  (use "git branch --unset-upstream" to fixup)

nothing to commit, working tree clean
$ git branch --unset-upstream
fatal: could not unset 'branch.yves/xxx.merge'
$ git status
On branch yves/xxx
nothing to commit, working tree clean
$ git fetch
fatal: No remote repository specified.  Please, specify either a URL or a
remote name from which new revisions should be fetched.

quoted

At this point my .git/config file was empty, and all of my config was lost.

I assume that things that rewrite .git/config do not check for a
successful write before deleting the old version of the file.

No, it writes the new content to "config.lock" and then renames it into
place.
All of the write() calls to the temporary file are checked.

I was going to say that perhaps the write was not checked... But if
you are confident they are then...

The
old data is copied over after having been ready by mmap (which is also
error-checked).

Given that your output is consistent with it failing to find the key,
and that the result is an empty file, it sounds like somehow the mmap'd
input appeared empty (but neither open nor fstat nor mmap returned an
error). You're not on any kind of exotic filesystem, are you?

I don't think so, but I don't know. Is there a command I can run to check?

BTW, with a bit of faffing I can probably recreate this problem.
Should I try? Is there something I could do during recreation that
would help?

cheers,
Yves

-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

On Wed, Sep 13, 2017 at 03:38:52PM +0200, demerphq wrote:

I just double checked the terminal history and this is all i saw:

$ git status
On branch yves/xxx
Your branch is based on 'origin/yves/xxx', but the upstream is gone.
  (use "git branch --unset-upstream" to fixup)

nothing to commit, working tree clean
$ git branch --unset-upstream
fatal: could not unset 'branch.yves/xxx.merge'
$ git status
On branch yves/xxx
nothing to commit, working tree clean
$ git fetch
fatal: No remote repository specified.  Please, specify either a URL or a
remote name from which new revisions should be fetched.

As a side note, I'm surprised that commands work at all when your
.git/config is empty. I'd expect check_respository_format() to complain
that you are not in a repository.  Looks like it is due to this block:

  452		 /*
  453		  * For historical use of check_repository_format() in git-init,
  454		  * we treat a missing config as a silent "ok", even when nongit_ok
  455		  * is unset.
  456		  */
  457		if (candidate.version < 0)
  458			return 0;

quoted

No, it writes the new content to "config.lock" and then renames it into
place.
All of the write() calls to the temporary file are checked.

I was going to say that perhaps the write was not checked... But if
you are confident they are then...

You're welcome to read over the function to double-check, but I just
looked it over and couldn't find any unchecked writes.

quoted

Given that your output is consistent with it failing to find the key,
and that the result is an empty file, it sounds like somehow the mmap'd
input appeared empty (but neither open nor fstat nor mmap returned an
error). You're not on any kind of exotic filesystem, are you?

I don't think so, but I don't know. Is there a command I can run to check?

BTW, with a bit of faffing I can probably recreate this problem.
Should I try? Is there something I could do during recreation that
would help?

If you think you can reproduce, the output of "strace" on a failing
invocation would be very interesting.

-Peff

On 13 September 2017 at 16:17, Jeff King [off-list ref] wrote:

You're welcome to read over the function to double-check, but I just
looked it over and couldn't find any unchecked writes.

I can look, but I doubt I would notice something you did not.

On the other hand the strace output does show that this is a case
where the writes failed, but we still renamed the empty config.lock
file into place:


write(3, "[core]\n\tsharedRepository = true\n"..., 288) = -1 ENOSPC
(No space left on device)
write(3, "        merge = refs/heads/yves/"..., 51) = -1 ENOSPC (No
space left on device)
munmap(0x7f48d9b8c000, 363)             = 0
close(3)                                = 0
rename("/usr/local/git_tree/main/.git/config.lock",
"/usr/local/git_tree/main/.git/config") = 0

Full strace is below:

quoted

quoted

Given that your output is consistent with it failing to find the key,
and that the result is an empty file, it sounds like somehow the mmap'd
input appeared empty (but neither open nor fstat nor mmap returned an
error). You're not on any kind of exotic filesystem, are you?

I don't think so, but I don't know. Is there a command I can run to check?

I freed up space and things worked, so I somehow doubt the filesystem
is at fault. When I then filled up the disk and retried the error was
repeatable.

quoted

BTW, with a bit of faffing I can probably recreate this problem.
Should I try? Is there something I could do during recreation that
would help?

If you think you can reproduce, the output of "strace" on a failing
invocation would be very interesting.

I can reproduce, see below. Preceded and suffixed by ls on the .git/config file.

I have munged the branch name for privacy reasons, hope that doesn't
invalidate the strace utility.

cheers,
yves

$ ls -la .git/config
-rw-rw-r-- 1 root users 363 Sep 13 16:36 .git/config
$ strace git branch --unset-upstream
execve("/usr/bin/git", ["git", "branch", "--unset-upstream"], [/* 39
vars */]) = 0
brk(0)                                  = 0x222c000
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7f48d9b94000
access("/etc/ld.so.preload", R_OK)      = -1 ENOENT (No such file or directory)
open("/etc/ld.so.cache", O_RDONLY)      = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=50300, ...}) = 0
mmap(NULL, 50300, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7f48d9b87000
close(3)                                = 0
open("/lib64/libpcre.so.0", O_RDONLY)   = 3
read(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"...,
832) = 832
fstat(3, {st_mode=S_IFREG|0755, st_size=183080, ...}) = 0
mmap(NULL, 2278264, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3,
0) = 0x7f48d9748000
mprotect(0x7f48d9774000, 2097152, PROT_NONE) = 0
mmap(0x7f48d9974000, 4096, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x2c000) = 0x7f48d9974000
close(3)                                = 0
open("/lib64/libz.so.1", O_RDONLY)      = 3
read(3, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0
!\0\0\0\0\0\0"..., 832) = 832
fstat(3, {st_mode=S_IFREG|0755, st_size=88600, ...}) = 0
mmap(NULL, 2183696, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3,
0) = 0x7f48d9532000
mprotect(0x7f48d9547000, 2093056, PROT_NONE) = 0
mmap(0x7f48d9746000, 8192, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x14000) = 0x7f48d9746000
close(3)                                = 0
open("/lib64/libpthread.so.0", O_RDONLY) = 3
read(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"...,
832) = 832
fstat(3, {st_mode=S_IFREG|0755, st_size=143280, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7f48d9b86000
mmap(NULL, 2212848, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3,
0) = 0x7f48d9315000
mprotect(0x7f48d932c000, 2097152, PROT_NONE) = 0
mmap(0x7f48d952c000, 8192, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x17000) = 0x7f48d952c000
mmap(0x7f48d952e000, 13296, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7f48d952e000
close(3)                                = 0
open("/lib64/librt.so.1", O_RDONLY)     = 3
read(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"...,
832) = 832
fstat(3, {st_mode=S_IFREG|0755, st_size=44472, ...}) = 0
mmap(NULL, 2128816, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3,
0) = 0x7f48d910d000
mprotect(0x7f48d9114000, 2093056, PROT_NONE) = 0
mmap(0x7f48d9313000, 8192, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x6000) = 0x7f48d9313000
close(3)                                = 0
open("/lib64/libc.so.6", O_RDONLY)      = 3
read(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"...,
832) = 832
fstat(3, {st_mode=S_IFREG|0755, st_size=1924768, ...}) = 0
mmap(NULL, 3750184, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3,
0) = 0x7f48d8d79000
mprotect(0x7f48d8f03000, 2097152, PROT_NONE) = 0
mmap(0x7f48d9103000, 24576, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x18a000) = 0x7f48d9103000
mmap(0x7f48d9109000, 14632, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7f48d9109000
close(3)                                = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7f48d9b85000
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7f48d9b84000
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7f48d9b83000
arch_prctl(ARCH_SET_FS, 0x7f48d9b84700) = 0
mprotect(0x7f48d9103000, 16384, PROT_READ) = 0
mprotect(0x7f48d9313000, 4096, PROT_READ) = 0
mprotect(0x7f48d952c000, 4096, PROT_READ) = 0
mprotect(0x7f48d9746000, 4096, PROT_READ) = 0
mprotect(0x7f48d9b95000, 4096, PROT_READ) = 0
munmap(0x7f48d9b87000, 50300)           = 0
set_tid_address(0x7f48d9b849d0)         = 50373
set_robust_list(0x7f48d9b849e0, 24)     = 0
futex(0x7fff059e43bc, FUTEX_WAKE_PRIVATE, 1) = 0
futex(0x7fff059e43bc, FUTEX_WAIT_BITSET_PRIVATE|FUTEX_CLOCK_REALTIME,
1, NULL, 7f48d9b84700) = -1 EAGAIN (Resource temporarily unavailable)
rt_sigaction(SIGRTMIN, {0x7f48d931acb0, [], SA_RESTORER|SA_SIGINFO,
0x7f48d93247e0}, NULL, 8) = 0
rt_sigaction(SIGRT_1, {0x7f48d931ad40, [],
SA_RESTORER|SA_RESTART|SA_SIGINFO, 0x7f48d93247e0}, NULL, 8) = 0
rt_sigprocmask(SIG_UNBLOCK, [RTMIN RT_1], NULL, 8) = 0
getrlimit(RLIMIT_STACK, {rlim_cur=10240*1024, rlim_max=RLIM64_INFINITY}) = 0
open("/dev/null", O_RDWR)               = 3
close(3)                                = 0
brk(0)                                  = 0x222c000
brk(0x224d000)                          = 0x224d000
open("/usr/lib/locale/locale-archive", O_RDONLY) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=99170352, ...}) = 0
mmap(NULL, 99170352, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7f48d2ee5000
close(3)                                = 0
open("/usr/lib64/gconv/gconv-modules.cache", O_RDONLY) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=26060, ...}) = 0
mmap(NULL, 26060, PROT_READ, MAP_SHARED, 3, 0) = 0x7f48d9b8d000
close(3)                                = 0
futex(0x7f48d9108828, FUTEX_WAKE_PRIVATE, 2147483647) = 0
rt_sigprocmask(SIG_UNBLOCK, [PIPE], NULL, 8) = 0
rt_sigaction(SIGPIPE, {SIG_DFL, [PIPE], SA_RESTORER|SA_RESTART,
0x7f48d8dab510}, {SIG_DFL, [], 0}, 8) = 0
getcwd("/usr/local/git_tree/main", 129) = 25
stat("/usr/local/git_tree/main", {st_mode=S_IFDIR|S_ISGID|0775,
st_size=4096, ...}) = 0
stat("/usr/local/git_tree/main/.git", {st_mode=S_IFDIR|S_ISGID|0775,
st_size=4096, ...}) = 0
lstat("/usr/local/git_tree/main/.git/HEAD", {st_mode=S_IFREG|0664,
st_size=39, ...}) = 0
open("/usr/local/git_tree/main/.git/HEAD", O_RDONLY) = 3
read(3, "ref: refs/heads/yves/simple_proj"..., 255) = 39
read(3, "", 216)                        = 0
close(3)                                = 0
lstat("/usr/local/git_tree/main/.git/commondir", 0x7fff059e3ec0) = -1
ENOENT (No such file or directory)
access("/usr/local/git_tree/main/.git/objects", X_OK) = 0
access("/usr/local/git_tree/main/.git/refs", X_OK) = 0
lstat(".git/commondir", 0x7fff059e3ef0) = -1 ENOENT (No such file or directory)
open(".git/config", O_RDONLY)           = 3
fstat(3, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0
fstat(3, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7f48d9b8c000
read(3, "[core]\n\tsharedRepository = true\n"..., 4096) = 363
read(3, "", 4096)                       = 0
close(3)                                = 0
munmap(0x7f48d9b8c000, 4096)            = 0
getcwd("/usr/local/git_tree/main", 129) = 25
stat(".git", {st_mode=S_IFDIR|S_ISGID|0775, st_size=4096, ...}) = 0
lstat(".git/commondir", 0x7fff059e3f50) = -1 ENOENT (No such file or directory)
access("/etc/gitconfig", R_OK)          = 0
open("/etc/gitconfig", O_RDONLY)        = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=1353, ...}) = 0
fstat(3, {st_mode=S_IFREG|0644, st_size=1353, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7f48d9b8c000
read(3, "################################"..., 4096) = 1353
read(3, "", 4096)                       = 0
close(3)                                = 0
munmap(0x7f48d9b8c000, 4096)            = 0
access("/home/yorton/.config/git/config", R_OK) = -1 ENOENT (No such
file or directory)
access("/home/yorton/.gitconfig", R_OK) = 0
open("/home/yorton/.gitconfig", O_RDONLY) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=377, ...}) = 0
fstat(3, {st_mode=S_IFREG|0644, st_size=377, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7f48d9b8c000
read(3, "[alias]\n\thist = log --graph --co"..., 4096) = 377
read(3, "", 4096)                       = 0
close(3)                                = 0
munmap(0x7f48d9b8c000, 4096)            = 0
access(".git/config", R_OK)             = 0
open(".git/config", O_RDONLY)           = 3
fstat(3, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0
fstat(3, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7f48d9b8c000
read(3, "[core]\n\tsharedRepository = true\n"..., 4096) = 363
read(3, "", 4096)                       = 0
close(3)                                = 0
munmap(0x7f48d9b8c000, 4096)            = 0
open("/usr/share/locale/locale.alias", O_RDONLY|O_CLOEXEC) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=2512, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7f48d9b8c000
read(3, "# Locale name alias data base.\n#"..., 4096) = 2512
read(3, "", 4096)                       = 0
close(3)                                = 0
munmap(0x7f48d9b8c000, 4096)            = 0
open("/usr/share/locale/en_CA.utf8/LC_MESSAGES/git.mo", O_RDONLY) = -1
ENOENT (No such file or directory)
open("/usr/share/locale/en_CA/LC_MESSAGES/git.mo", O_RDONLY) = -1
ENOENT (No such file or directory)
open("/usr/share/locale/en.utf8/LC_MESSAGES/git.mo", O_RDONLY) = -1
ENOENT (No such file or directory)
open("/usr/share/locale/en/LC_MESSAGES/git.mo", O_RDONLY) = -1 ENOENT
(No such file or directory)
access("/etc/gitconfig", R_OK)          = 0
open("/etc/gitconfig", O_RDONLY)        = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=1353, ...}) = 0
fstat(3, {st_mode=S_IFREG|0644, st_size=1353, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7f48d9b8c000
read(3, "################################"..., 4096) = 1353
read(3, "", 4096)                       = 0
close(3)                                = 0
munmap(0x7f48d9b8c000, 4096)            = 0
access("/home/yorton/.config/git/config", R_OK) = -1 ENOENT (No such
file or directory)
access("/home/yorton/.gitconfig", R_OK) = 0
open("/home/yorton/.gitconfig", O_RDONLY) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=377, ...}) = 0
fstat(3, {st_mode=S_IFREG|0644, st_size=377, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7f48d9b8c000
read(3, "[alias]\n\thist = log --graph --co"..., 4096) = 377
read(3, "", 4096)                       = 0
close(3)                                = 0
munmap(0x7f48d9b8c000, 4096)            = 0
access(".git/config", R_OK)             = 0
open(".git/config", O_RDONLY)           = 3
fstat(3, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0
fstat(3, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7f48d9b8c000
read(3, "[core]\n\tsharedRepository = true\n"..., 4096) = 363
read(3, "", 4096)                       = 0
close(3)                                = 0
munmap(0x7f48d9b8c000, 4096)            = 0
lstat(".git/commondir", 0x7fff059e3590) = -1 ENOENT (No such file or directory)
lstat(".git/HEAD", {st_mode=S_IFREG|0664, st_size=39, ...}) = 0
open(".git/HEAD", O_RDONLY)             = 3
read(3, "ref: refs/heads/yves/xxx"..., 256) = 39
read(3, "", 217)                        = 0
close(3)                                = 0
lstat(".git/refs/heads/yves/xxx", 0x7fff059e3580) = -1 ENOENT (No such
file or directory)
open(".git/packed-refs", O_RDONLY)      = 3
fstat(3, {st_mode=S_IFREG|0664, st_size=231354, ...}) = 0
fstat(3, {st_mode=S_IFREG|0664, st_size=231354, ...}) = 0
fstat(3, {st_mode=S_IFREG|0664, st_size=231354, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7f48d9b8c000
read(3, "# pack-refs with: peeled fully-p"..., 4096) = 4096
read(3, "1a5c122f68f04fae374ac3cdf11\n14e0"..., 4096) = 4096
read(3, "b16f20dde39f97ef2869296\nba29d061"..., 4096) = 4096
read(3, "6-141414\n^29ba731b77bb5f5c15644d"..., 4096) = 4096
read(3, "df2d5e\n3cd1ca6cffc2ce48ecc09c0c1"..., 4096) = 4096
read(3, "fd414d360b4d742e8711fe8093a09fa "..., 4096) = 4096
read(3, "pit-20170511-164552\n^cb28295c135"..., 4096) = 4096
read(3, "4a4cb2cf8620771feb\n41b2f890ea48b"..., 4096) = 4096
read(3, "\n^9827bb20ed16ec66dd510d72d779c6"..., 4096) = 4096
read(3, "-160507\n^56472ff6a42c58dee27d337"..., 4096) = 4096
read(3, "0d01a69f3c0\nef9052d4b9c0e143a5f0"..., 4096) = 4096
read(3, "/tags/dummyapp-20170609-182243_a"..., 4096) = 4096
read(3, "5418485a012396832faedc5\nc697717c"..., 4096) = 4096
read(3, "3-141556\n^69219f5ddd74c43d5574e5"..., 4096) = 4096
read(3, "0602-122026\n^4cbb642a2b3cb305336"..., 4096) = 4096
read(3, "a9bee28a76502716da588fd2fc22020a"..., 4096) = 4096
read(3, "73a677f refs/tags/events-2016090"..., 4096) = 4096
read(3, "3be9fd87d5bf6217aaf195d5\n650866a"..., 4096) = 4096
read(3, "a95cda22bc90a79\n4f3a36e9adac266f"..., 4096) = 4096
read(3, "ec1097435aeed7ed8\n7a2c97e8ad1a9f"..., 4096) = 4096
read(3, "dba7614549cec3ed80803b6b01161c3d"..., 4096) = 4096
read(3, "4ef8e8f\n7e3c15cd86655da442c48ba7"..., 4096) = 4096
read(3, "1d057a4b7dc47ac09f40a8d31916218\n"..., 4096) = 4096
read(3, "s/tags/intercomcron-20170615-110"..., 4096) = 4096
read(3, "\n^43f080925840cf77ae016fa93ecbdf"..., 4096) = 4096
read(3, "gs/logproc-20161230-121402\n^a25f"..., 4096) = 4096
read(3, "2d55c54c708fca2cfe726e72d86ad50 "..., 4096) = 4096
read(3, "70612-163608\n^614fcefc7f4c39b12f"..., 4096) = 4096
read(3, "ilebuild-20170512-145808\n^47a87c"..., 4096) = 4096
brk(0x226e000)                          = 0x226e000
read(3, "7a428de652f5a3c9c4\n214d8d1128260"..., 4096) = 4096
read(3, "\n^e5486659e4bad48a3fb1177b619f11"..., 4096) = 4096
read(3, "91f33991c3efed545f51 refs/tags/p"..., 4096) = 4096
read(3, "08d313fb454\nefe6a551adac01f51538"..., 4096) = 4096
read(3, "/ppcapp-20170628-111257\n^f56019d"..., 4096) = 4096
read(3, "b50901 refs/tags/provdash-201706"..., 4096) = 4096
read(3, "7 refs/tags/rtmapp-20170613-1102"..., 4096) = 4096
read(3, "-192153\n^296cc5ffc1c84b049fd0a15"..., 4096) = 4096
read(3, "a6\nda231495f5708ecdafe41aedf28a4"..., 4096) = 4096
read(3, "202\n^70ebec46ba2859c12b67a6dca82"..., 4096) = 4096
read(3, "9d54a38eeff507e45d\n9d93a8eaf70d3"..., 4096) = 4096
read(3, "ecure-hmsapp-20161021-181248\n^a5"..., 4096) = 4096
read(3, "0111-134153\n^13d748af46df47b5315"..., 4096) = 4096
read(3, "1 refs/tags/soylent-20170606-154"..., 4096) = 4096
read(3, "tags/statprofileapp-20161104-125"..., 4096) = 4096
read(3, "tags/substreamproc-20170214-1429"..., 4096) = 4096
read(3, "efs/tags/suite_partner_sites-201"..., 4096) = 4096
read(3, "167 refs/tags/tokenauth-20160929"..., 4096) = 4096
read(3, "640\n^00b52cdaaaf3c55e069cb62740d"..., 4096) = 4096
read(3, "9753e496ebbf7b05f009dd32\ndb917ed"..., 4096) = 4096
read(3, "8cc96a8a9313d82cc4c91d2ddc31613b"..., 4096) = 4096
read(3, "2ad87b80a7a0\n56519d1a3c4fe5cdcc3"..., 4096) = 4096
read(3, " refs/tags/xml-secure-mobile-201"..., 4096) = 4096
read(3, "794a5b25d4b5d411cc736298a\nb4cb1c"..., 4096) = 4096
read(3, "057d938c27f8b16e6ef9839874f696a2"..., 4096) = 4096
read(3, "3f3259ffc0262e4bf56f4eec01c76ac8"..., 4096) = 4096
read(3, "\n^dbab804616296abf788d4a1d991b3b"..., 4096) = 4096
read(3, "dad60d1e168e7d7de94e3da1 refs/ta"..., 4096) = 1978
read(3, "", 4096)                       = 0
close(3)                                = 0
munmap(0x7f48d9b8c000, 4096)            = 0
lstat(".git/HEAD", {st_mode=S_IFREG|0664, st_size=39, ...}) = 0
open(".git/HEAD", O_RDONLY)             = 3
read(3, "ref: refs/heads/yves/xxx"..., 256) = 39
read(3, "", 217)                        = 0
close(3)                                = 0
lstat(".git/refs/heads/yves/xxx", 0x7fff059e34b0) = -1 ENOENT (No such
file or directory)
stat(".git/packed-refs", {st_mode=S_IFREG|0664, st_size=231354, ...}) = 0
readlink(".git/config", 0x2267b40, 32)  = -1 EINVAL (Invalid argument)
rt_sigaction(SIGINT, {0x54a550, [INT], SA_RESTORER|SA_RESTART,
0x7f48d8dab510}, {SIG_DFL, [], 0}, 8) = 0
rt_sigaction(SIGHUP, {0x54a550, [HUP], SA_RESTORER|SA_RESTART,
0x7f48d8dab510}, {SIG_DFL, [], 0}, 8) = 0
rt_sigaction(SIGTERM, {0x54a550, [TERM], SA_RESTORER|SA_RESTART,
0x7f48d8dab510}, {SIG_DFL, [], 0}, 8) = 0
rt_sigaction(SIGQUIT, {0x54a550, [QUIT], SA_RESTORER|SA_RESTART,
0x7f48d8dab510}, {SIG_DFL, [], 0}, 8) = 0
rt_sigaction(SIGPIPE, {0x54a550, [PIPE], SA_RESTORER|SA_RESTART,
0x7f48d8dab510}, {SIG_DFL, [PIPE], SA_RESTORER|SA_RESTART,
0x7f48d8dab510}, 8) = 0
getcwd("/usr/local/git_tree/main", 129) = 25
open("/usr/local/git_tree/main/.git/config.lock",
O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3
lstat("/usr/local/git_tree/main/.git/config.lock",
{st_mode=S_IFREG|0664, st_size=0, ...}) = 0
open(".git/config", O_RDONLY)           = 4
open(".git/config", O_RDONLY)           = 5
fstat(5, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0
fstat(5, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7f48d9b8c000
read(5, "[core]\n\tsharedRepository = true\n"..., 4096) = 363
lseek(5, 0, SEEK_CUR)                   = 363
read(5, "", 4096)                       = 0
close(5)                                = 0
munmap(0x7f48d9b8c000, 4096)            = 0
fstat(4, {st_mode=S_IFREG|0664, st_size=363, ...}) = 0
mmap(NULL, 363, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7f48d9b8c000
close(4)                                = 0
chmod("/usr/local/git_tree/main/.git/config.lock", 0664) = 0
write(3, "[core]\n\tsharedRepository = true\n"..., 288) = -1 ENOSPC
(No space left on device)
write(3, "        merge = refs/heads/yves/"..., 51) = -1 ENOSPC (No
space left on device)
munmap(0x7f48d9b8c000, 363)             = 0
close(3)                                = 0
rename("/usr/local/git_tree/main/.git/config.lock",
"/usr/local/git_tree/main/.git/config") = 0
readlink(".git/config", 0x2230d00, 32)  = -1 EINVAL (Invalid argument)
getcwd("/usr/local/git_tree/main", 129) = 25
open("/usr/local/git_tree/main/.git/config.lock",
O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3
lstat("/usr/local/git_tree/main/.git/config.lock",
{st_mode=S_IFREG|0664, st_size=0, ...}) = 0
open(".git/config", O_RDONLY)           = 4
open(".git/config", O_RDONLY)           = 5
fstat(5, {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
fstat(5, {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0x7f48d9b8c000
read(5, "", 4096)                       = 0
close(5)                                = 0
munmap(0x7f48d9b8c000, 4096)            = 0
close(3)                                = 0
unlink("/usr/local/git_tree/main/.git/config.lock") = 0
close(4)                                = 0
write(2, "fatal: could not unset 'branch.y"..., 61fatal: could not
unset 'branch.yves/xxx.merge'
) = 61
exit_group(128)                         = ?

+++ exited with 128 +++

$ ls -la .git/config
-rw-rw-r-- 1 yorton users 0 Sep 13 16:38 .git/config



-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

On Wed, Sep 13, 2017 at 04:49:45PM +0200, demerphq wrote:

On 13 September 2017 at 16:17, Jeff King [off-list ref] wrote:

quoted

You're welcome to read over the function to double-check, but I just
looked it over and couldn't find any unchecked writes.

I can look, but I doubt I would notice something you did not.

On the other hand the strace output does show that this is a case
where the writes failed, but we still renamed the empty config.lock
file into place:


write(3, "[core]\n\tsharedRepository = true\n"..., 288) = -1 ENOSPC
(No space left on device)
write(3, "        merge = refs/heads/yves/"..., 51) = -1 ENOSPC (No
space left on device)
munmap(0x7f48d9b8c000, 363)             = 0
close(3)                                = 0
rename("/usr/local/git_tree/main/.git/config.lock",
"/usr/local/git_tree/main/.git/config") = 0

Hmph. That is very disturbing. But with that information I should be
able to track down the culprit. Thanks for digging.

I freed up space and things worked, so I somehow doubt the filesystem
is at fault. When I then filled up the disk and retried the error was
repeatable.

Yeah, agreed. This really does look like a bug.

-Peff

On 13 September 2017 at 16:51, Jeff King [off-list ref] wrote:

On Wed, Sep 13, 2017 at 04:49:45PM +0200, demerphq wrote:

quoted

On 13 September 2017 at 16:17, Jeff King [off-list ref] wrote:

quoted

You're welcome to read over the function to double-check, but I just
looked it over and couldn't find any unchecked writes.

I can look, but I doubt I would notice something you did not.

On the other hand the strace output does show that this is a case
where the writes failed, but we still renamed the empty config.lock
file into place:


write(3, "[core]\n\tsharedRepository = true\n"..., 288) = -1 ENOSPC
(No space left on device)
write(3, "        merge = refs/heads/yves/"..., 51) = -1 ENOSPC (No
space left on device)
munmap(0x7f48d9b8c000, 363)             = 0
close(3)                                = 0
rename("/usr/local/git_tree/main/.git/config.lock",
"/usr/local/git_tree/main/.git/config") = 0

Hmph. That is very disturbing. But with that information I should be
able to track down the culprit. Thanks for digging.

FWIW, I see that git_config_set_multivar_in_file_gently() uses
write_in_full() which in turn uses xwrite(), but the latter has the
following comment on it:

/*
 * xwrite() is the same a write(), but it automatically restarts write()
 * operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT
 * GUARANTEE that "len" bytes is written even if the operation is successful.
 */

I suspect that at this point I am not adding much value here, so I
will leave it at this.

quoted

I freed up space and things worked, so I somehow doubt the filesystem
is at fault. When I then filled up the disk and retried the error was
repeatable.

Yeah, agreed. This really does look like a bug.

FWIW, where it bit me turned out to be harmless. So while no doubt
this could be a real PITA for someone it wasn't for me.

Yves


-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

On Wed, Sep 13, 2017 at 05:18:56PM +0200, demerphq wrote:

quoted

Hmph. That is very disturbing. But with that information I should be
able to track down the culprit. Thanks for digging.

FWIW, I see that git_config_set_multivar_in_file_gently() uses
write_in_full() which in turn uses xwrite(), but the latter has the
following comment on it:

/*
 * xwrite() is the same a write(), but it automatically restarts write()
 * operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT
 * GUARANTEE that "len" bytes is written even if the operation is successful.
 */

I suspect that at this point I am not adding much value here, so I
will leave it at this.

No, the problem is in this line:

                                 if (write_in_full(fd, contents + copy_begin,
                                                   copy_end - copy_begin) <
                                     copy_end - copy_begin)
                                          goto write_err_out;

write_in_full() returns -1 on error (_not_ how many bytes were actually
written). So its return is a signed ssize_t. But the result of the
pointer subtraction "copy_end - copy_begin" is an unsigned ptrdiff_t.
The compiler promotes the signed to an unsigned, so the condition can
never be true (the "-1" becomes the highest possible value).

I have the fix, but I'm searching the code base for other instances of
the same error.

-Peff

On 13 September 2017 at 17:22, Jeff King [off-list ref] wrote:

On Wed, Sep 13, 2017 at 05:18:56PM +0200, demerphq wrote:

quoted

quoted

Hmph. That is very disturbing. But with that information I should be
able to track down the culprit. Thanks for digging.

FWIW, I see that git_config_set_multivar_in_file_gently() uses
write_in_full() which in turn uses xwrite(), but the latter has the
following comment on it:

/*
 * xwrite() is the same a write(), but it automatically restarts write()
 * operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT
 * GUARANTEE that "len" bytes is written even if the operation is successful.
 */

I suspect that at this point I am not adding much value here, so I
will leave it at this.

No, the problem is in this line:

                                 if (write_in_full(fd, contents + copy_begin,
                                                   copy_end - copy_begin) <
                                     copy_end - copy_begin)
                                          goto write_err_out;

write_in_full() returns -1 on error (_not_ how many bytes were actually
written). So its return is a signed ssize_t. But the result of the
pointer subtraction "copy_end - copy_begin" is an unsigned ptrdiff_t.
The compiler promotes the signed to an unsigned, so the condition can
never be true (the "-1" becomes the highest possible value).

Bah. Good eye. I missed that entirely.

I have the fix, but I'm searching the code base for other instances of
the same error.

Yeah, I think there are few just in that file.

Thanks for fixing this!

cheers,
Yves

-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

On Wed, Sep 13, 2017 at 01:59:17PM +0200, demerphq wrote:

After being away for a while I saw the following message in one of my git repos:

$ git status
On branch yves/xxx
Your branch is based on 'origin/yves/xxx', but the upstream is gone.
  (use "git branch --unset-upstream" to fixup)

nothing to commit, working tree clean
$ git branch --unset-upstream
fatal: could not unset 'branch.yves/simple_projection.merge'

At this point my .git/config file was empty, and all of my config was lost.

Here's the fix, along with some related cleanups. The actual bugfix is
in the first patch, which I think should go to "maint". The rest are not
as important, so could go to master (but would also be fine for maint,
as there should be no user-visible changes).

  [1/7]: config: avoid "write_in_full(fd, buf, len) < len" pattern
  [2/7]: get-tar-commit-id: check write_in_full() return against 0
  [3/7]: avoid "write_in_full(fd, buf, len) != len" pattern
  [4/7]: convert less-trivial versions of "write_in_full() != len"
  [5/7]: pkt-line: check write_in_full() errors against "< 0"
  [6/7]: notes-merge: use ssize_t for write_in_full() return value
  [7/7]: config: flip return value of store_write_*()

 builtin/get-tar-commit-id.c |  3 +--
 builtin/receive-pack.c      |  2 +-
 builtin/rerere.c            |  2 +-
 builtin/unpack-file.c       |  2 +-
 config.c                    | 38 +++++++++++++++++++-------------------
 diff.c                      |  2 +-
 entry.c                     |  5 +++--
 fast-import.c               |  2 +-
 http-backend.c              |  4 ++--
 ll-merge.c                  |  2 +-
 notes-merge.c               |  2 +-
 pkt-line.c                  | 29 ++++++++++++++---------------
 read-cache.c                |  6 +++---
 refs.c                      |  2 +-
 refs/files-backend.c        | 10 +++++-----
 rerere.c                    |  2 +-
 shallow.c                   |  6 +++---
 streaming.c                 |  2 +-
 t/helper/test-delta.c       |  2 +-
 transport-helper.c          |  5 ++---
 wrapper.c                   |  2 +-
 21 files changed, 64 insertions(+), 66 deletions(-)

-Peff

The return type of write_in_full() is a signed ssize_t,
because we may return "-1" on failure (even if we succeeded
in writing some bytes). But "len" itself may be an
unsigned type (the function takes a size_t, but of course we
may have something else in the calling function). So while
it seems like:

  if (write_in_full(fd, buf, len) < len)
	die_errno("write error");

would notice an error, it won't if "len" is unsigned.  The
compiler sees a signed/unsigned comparison and promotes the
signed value, resulting in (size_t)-1, the highest possible
size_t (or again, whatever type the caller has). This cannot
possibly be smaller than "len", and so the conditional can
never trigger.

I scoured the code base for cases of this, but it turns out
that these two in git_config_set_multivar_in_file_gently()
are the only ones. This case is actually quite interesting:
we don't have a size_t, but rather use the subtraction of
two pointers. Which you might think would be a signed
ptrdiff_t, but clearly both gcc and clang treat it as
unsigned (possibly because the conditional just above
guarantees that the result is greater than zero).

We can avoid the whole question by just checking for a
negative return value directly, as write_in_full() will
never return any value except -1 or the full count.

There's no addition to the test suite here, since you need
to convince write() to fail in order to see the problem. The
simplest reproduction recipe I came up with is to trigger
ENOSPC (this only works on Linux, obviously):

  # make a limited-size filesystem
  dd if=/dev/zero of=small.disk bs=1M count=1
  mke2fs small.disk
  mkdir mnt
  sudo mount -o loop small.disk mnt
  cd mnt
  sudo chown $USER:$USER .

  # make a config file with some content
  git config --file=config one.key value
  git config --file=config two.key value

  # now fill up the disk
  dd if=/dev/zero of=fill

  # and try to delete a key, which requires copying the rest
  # of the file to config.lock, and will fail on write()
  git config --file=config --unset two.key

That final command should (and does after this patch)
produce an error message due to the failed write, and leave
the file intact. With the current code it silently ignores
the failure and renames config.lock into place, leaving you
with a totally empty config file!

Reported-by: demerphq <redacted>
Signed-off-by: Jeff King <redacted>
---
 config.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/config.c b/config.c
index d0d8ce823a..eee4ac0355 100644
--- a/config.c
+++ b/config.c

@@ -2608,8 +2608,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 			/* write the first part of the config */
 			if (copy_end > copy_begin) {
 				if (write_in_full(fd, contents + copy_begin,
-						  copy_end - copy_begin) <
-				    copy_end - copy_begin)
+						  copy_end - copy_begin) < 0)
 					goto write_err_out;
 				if (new_line &&
 				    write_str_in_full(fd, "\n") != 1)

@@ -2631,8 +2630,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 		/* write the rest of the config */
 		if (copy_begin < contents_sz)
 			if (write_in_full(fd, contents + copy_begin,
-					  contents_sz - copy_begin) <
-			    contents_sz - copy_begin)
+					  contents_sz - copy_begin) < 0)
 				goto write_err_out;
 
 		munmap(contents, contents_sz);

-- 
2.14.1.874.ge7b2e05270

We ask to write 41 bytes and make sure that the return value
is at least 41. This is the same "dangerous" pattern that
was fixed in the prior commit (wherein a negative return
value is promoted to unsigned), though it is not dangerous
here because our "41" is a constant, not an unsigned
variable.

But we should convert it anyway to avoid modeling a
dangerous construct.

Signed-off-by: Jeff King <redacted>
---
 builtin/get-tar-commit-id.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c
index e21c5416cd..6d9a79f9b3 100644
--- a/builtin/get-tar-commit-id.c
+++ b/builtin/get-tar-commit-id.c

@@ -33,8 +33,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix)
 	if (!skip_prefix(content, "52 comment=", &comment))
 		return 1;
 
-	n = write_in_full(1, comment, 41);
-	if (n < 41)
+	if (write_in_full(1, comment, 41) < 0)
 		die_errno("git get-tar-commit-id: write error");
 
 	return 0;

-- 
2.14.1.874.ge7b2e05270

The return value of write_in_full() is either "-1", or the
requested number of bytes[1]. If we make a partial write
before seeing an error, we still return -1, not a partial
value. This goes back to f6aa66cb95 (write_in_full: really
write in full or return error on disk full., 2007-01-11).

So checking anything except "was the return value negative"
is pointless. And there are a couple of reasons not to do
so:

  1. It can do a funny signed/unsigned comparison. If your
     "len" is signed (e.g., a size_t) then the compiler will
     promote the "-1" to its unsigned variant.

     This works out for "!= len" (unless you really were
     trying to write the maximum size_t bytes), but is a
     bug if you check "< len" (an example of which was fixed
     recently in config.c).

     We should avoid promoting the mental model that you
     need to check the length at all, so that new sites are
     not tempted to copy us.

  2. Checking for a negative value is shorter to type,
     especially when the length is an expression.

  3. Linus says so. In d34cf19b89 (Clean up write_in_full()
     users, 2007-01-11), right after the write_in_full()
     semantics were changed, he wrote:

       I really wish every "write_in_full()" user would just
       check against "<0" now, but this fixes the nasty and
       stupid ones.

     Appeals to authority aside, this makes it clear that
     writing it this way does not have an intentional
     benefit. It's a historical curiosity that we never
     bothered to clean up (and which was undoubtedly
     cargo-culted into new sites).

So let's convert these obviously-correct cases (this
includes write_str_in_full(), which is just a wrapper for
write_in_full()).

[1] A careful reader may notice there is one way that
    write_in_full() can return a different value. If we ask
    write() to write N bytes and get a return value that is
    _larger_ than N, we could return a larger total. But
    besides the fact that this would imply a totally broken
    version of write(), it would already invoke undefined
    behavior. Our internal remaining counter is an unsigned
    size_t, which means that subtracting too many byte will
    wrap it around to a very large number. So we'll instantly
    begin reading off the end of the buffer, trying to write
    gigabytes (or petabytes) of data.

Signed-off-by: Jeff King <redacted>
---
 builtin/receive-pack.c | 2 +-
 builtin/rerere.c       | 2 +-
 builtin/unpack-file.c  | 2 +-
 config.c               | 4 ++--
 diff.c                 | 2 +-
 fast-import.c          | 2 +-
 http-backend.c         | 4 ++--
 ll-merge.c             | 2 +-
 read-cache.c           | 6 +++---
 refs.c                 | 2 +-
 refs/files-backend.c   | 8 ++++----
 rerere.c               | 2 +-
 shallow.c              | 6 +++---
 t/helper/test-delta.c  | 2 +-
 transport-helper.c     | 5 ++---
 wrapper.c              | 2 +-
 16 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 52c63ebfdc..dd06b3fb4f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c

@@ -743,7 +743,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
 		size_t n;
 		if (feed(feed_state, &buf, &n))
 			break;
-		if (write_in_full(proc.in, buf, n) != n)
+		if (write_in_full(proc.in, buf, n) < 0)
 			break;
 	}
 	close(proc.in);

diff --git a/builtin/rerere.c b/builtin/rerere.c
index ffb66e2907..0bc40298c2 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c

@@ -18,7 +18,7 @@ static int outf(void *dummy, mmbuffer_t *ptr, int nbuf)
 {
 	int i;
 	for (i = 0; i < nbuf; i++)
-		if (write_in_full(1, ptr[i].ptr, ptr[i].size) != ptr[i].size)
+		if (write_in_full(1, ptr[i].ptr, ptr[i].size) < 0)
 			return -1;
 	return 0;
 }

diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c
index 281ca1db6c..32e0155577 100644
--- a/builtin/unpack-file.c
+++ b/builtin/unpack-file.c

@@ -15,7 +15,7 @@ static char *create_temp_file(struct object_id *oid)
 
 	xsnprintf(path, sizeof(path), ".merge_file_XXXXXX");
 	fd = xmkstemp(path);
-	if (write_in_full(fd, buf, size) != size)
+	if (write_in_full(fd, buf, size) < 0)
 		die_errno("unable to write temp-file");
 	close(fd);
 	return path;

diff --git a/config.c b/config.c
index eee4ac0355..daf093db45 100644
--- a/config.c
+++ b/config.c

@@ -2611,7 +2611,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 						  copy_end - copy_begin) < 0)
 					goto write_err_out;
 				if (new_line &&
-				    write_str_in_full(fd, "\n") != 1)
+				    write_str_in_full(fd, "\n") < 0)
 					goto write_err_out;
 			}
 			copy_begin = store.offset[i];

@@ -2842,7 +2842,7 @@ int git_config_rename_section_in_file(const char *config_filename,
 		if (remove)
 			continue;
 		length = strlen(output);
-		if (write_in_full(out_fd, output, length) != length) {
+		if (write_in_full(out_fd, output, length) < 0) {
 			ret = write_error(get_lock_file_path(lock));
 			goto out;
 		}

diff --git a/diff.c b/diff.c
index 3d3e553a98..3914830ff1 100644
--- a/diff.c
+++ b/diff.c

@@ -3738,7 +3738,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
 		blob = buf.buf;
 		size = buf.len;
 	}
-	if (write_in_full(fd, blob, size) != size)
+	if (write_in_full(fd, blob, size) < 0)
 		die_errno("unable to write temp-file");
 	close_tempfile(&temp->tempfile);
 	temp->name = get_tempfile_path(&temp->tempfile);

diff --git a/fast-import.c b/fast-import.c
index 49516d60e6..35bf671f12 100644
--- a/fast-import.c
+++ b/fast-import.c

@@ -2952,7 +2952,7 @@ static void parse_reset_branch(const char *arg)
 
 static void cat_blob_write(const char *buf, unsigned long size)
 {
-	if (write_in_full(cat_blob_fd, buf, size) != size)
+	if (write_in_full(cat_blob_fd, buf, size) < 0)
 		die_errno("Write to frontend failed");
 }
 

diff --git a/http-backend.c b/http-backend.c
index 8076b1d5e5..e51c7805c8 100644
--- a/http-backend.c
+++ b/http-backend.c

@@ -358,7 +358,7 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)
 				die("zlib error inflating request, result %d", ret);
 
 			n = stream.total_out - cnt;
-			if (write_in_full(out, out_buf, n) != n)
+			if (write_in_full(out, out_buf, n) < 0)
 				die("%s aborted reading request", prog_name);
 			cnt += n;
 

@@ -379,7 +379,7 @@ static void copy_request(const char *prog_name, int out)
 	ssize_t n = read_request(0, &buf);
 	if (n < 0)
 		die_errno("error reading request body");
-	if (write_in_full(out, buf, n) != n)
+	if (write_in_full(out, buf, n) < 0)
 		die("%s aborted reading request", prog_name);
 	close(out);
 	free(buf);

diff --git a/ll-merge.c b/ll-merge.c
index 9fb855a900..a6ad2ec12d 100644
--- a/ll-merge.c
+++ b/ll-merge.c

@@ -154,7 +154,7 @@ static void create_temp(mmfile_t *src, char *path, size_t len)
 
 	xsnprintf(path, len, ".merge_file_XXXXXX");
 	fd = xmkstemp(path);
-	if (write_in_full(fd, src->ptr, src->size) != src->size)
+	if (write_in_full(fd, src->ptr, src->size) < 0)
 		die_errno("unable to write temp-file");
 	close(fd);
 }

diff --git a/read-cache.c b/read-cache.c
index 40da87ea71..357cea7a15 100644
--- a/read-cache.c
+++ b/read-cache.c

@@ -1922,7 +1922,7 @@ static int ce_write_flush(git_SHA_CTX *context, int fd)
 	unsigned int buffered = write_buffer_len;
 	if (buffered) {
 		git_SHA1_Update(context, write_buffer, buffered);
-		if (write_in_full(fd, write_buffer, buffered) != buffered)
+		if (write_in_full(fd, write_buffer, buffered) < 0)
 			return -1;
 		write_buffer_len = 0;
 	}

@@ -1971,7 +1971,7 @@ static int ce_flush(git_SHA_CTX *context, int fd, unsigned char *sha1)
 
 	/* Flush first if not enough space for SHA1 signature */
 	if (left + 20 > WRITE_BUFFER_SIZE) {
-		if (write_in_full(fd, write_buffer, left) != left)
+		if (write_in_full(fd, write_buffer, left) < 0)
 			return -1;
 		left = 0;
 	}

@@ -1980,7 +1980,7 @@ static int ce_flush(git_SHA_CTX *context, int fd, unsigned char *sha1)
 	git_SHA1_Final(write_buffer + left, context);
 	hashcpy(sha1, write_buffer + left);
 	left += 20;
-	return (write_in_full(fd, write_buffer, left) != left) ? -1 : 0;
+	return (write_in_full(fd, write_buffer, left) < 0) ? -1 : 0;
 }
 
 static void ce_smudge_racily_clean_entry(struct cache_entry *ce)

diff --git a/refs.c b/refs.c
index b0106b8162..30201cf0cb 100644
--- a/refs.c
+++ b/refs.c

@@ -627,7 +627,7 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
 		}
 	}
 
-	if (write_in_full(fd, buf.buf, buf.len) != buf.len) {
+	if (write_in_full(fd, buf.buf, buf.len) < 0) {
 		strbuf_addf(err, "could not write to '%s'", filename);
 		rollback_lock_file(&lock);
 		goto done;

diff --git a/refs/files-backend.c b/refs/files-backend.c
index fccbc24ac4..f8b91fff3f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -1628,8 +1628,8 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
 		return -1;
 	}
 	fd = get_lock_file_fd(lock->lk);
-	if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
-	    write_in_full(fd, &term, 1) != 1 ||
+	if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) < 0 ||
+	    write_in_full(fd, &term, 1) < 0 ||
 	    close_ref(lock) < 0) {
 		strbuf_addf(err,
 			    "couldn't write '%s'", get_lock_file_path(lock->lk));

@@ -2853,8 +2853,8 @@ static int files_reflog_expire(struct ref_store *ref_store,
 					strerror(errno));
 		} else if (update &&
 			   (write_in_full(get_lock_file_fd(lock->lk),
-				oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
-			    write_str_in_full(get_lock_file_fd(lock->lk), "\n") != 1 ||
+				oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) < 0 ||
+			    write_str_in_full(get_lock_file_fd(lock->lk), "\n") < 0 ||
 			    close_ref(lock) < 0)) {
 			status |= error("couldn't write %s",
 					get_lock_file_path(lock->lk));

diff --git a/rerere.c b/rerere.c
index d77235645e..1ce440f4bb 100644
--- a/rerere.c
+++ b/rerere.c

@@ -258,7 +258,7 @@ static int write_rr(struct string_list *rr, int out_fd)
 				    rerere_id_hex(id),
 				    rr->items[i].string, 0);
 
-		if (write_in_full(out_fd, buf.buf, buf.len) != buf.len)
+		if (write_in_full(out_fd, buf.buf, buf.len) < 0)
 			die("unable to write rerere record");
 
 		strbuf_release(&buf);

diff --git a/shallow.c b/shallow.c
index f5591e56da..57b494ab58 100644
--- a/shallow.c
+++ b/shallow.c

@@ -296,7 +296,7 @@ const char *setup_temporary_shallow(const struct oid_array *extra)
 	if (write_shallow_commits(&sb, 0, extra)) {
 		fd = xmks_tempfile(&temporary_shallow, git_path("shallow_XXXXXX"));
 
-		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
+		if (write_in_full(fd, sb.buf, sb.len) < 0)
 			die_errno("failed to write to %s",
 				  get_tempfile_path(&temporary_shallow));
 		close_tempfile(&temporary_shallow);

@@ -321,7 +321,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,
 				       LOCK_DIE_ON_ERROR);
 	check_shallow_file_for_update();
 	if (write_shallow_commits(&sb, 0, extra)) {
-		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
+		if (write_in_full(fd, sb.buf, sb.len) < 0)
 			die_errno("failed to write to %s",
 				  get_lock_file_path(shallow_lock));
 		*alternate_shallow_file = get_lock_file_path(shallow_lock);

@@ -368,7 +368,7 @@ void prune_shallow(int show_only)
 				       LOCK_DIE_ON_ERROR);
 	check_shallow_file_for_update();
 	if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {
-		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
+		if (write_in_full(fd, sb.buf, sb.len) < 0)
 			die_errno("failed to write to %s",
 				  get_lock_file_path(&shallow_lock));
 		commit_lock_file(&shallow_lock);

diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c
index 59937dc1be..591730adc4 100644
--- a/t/helper/test-delta.c
+++ b/t/helper/test-delta.c

@@ -69,7 +69,7 @@ int cmd_main(int argc, const char **argv)
 	}
 
 	fd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);
-	if (fd < 0 || write_in_full(fd, out_buf, out_size) != out_size) {
+	if (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {
 		perror(argv[4]);
 		return 1;
 	}

diff --git a/transport-helper.c b/transport-helper.c
index f50b34df2d..b66cb7d8e2 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -44,8 +44,7 @@ static void sendline(struct helper_data *helper, struct strbuf *buffer)
 {
 	if (debug)
 		fprintf(stderr, "Debug: Remote helper: -> %s", buffer->buf);
-	if (write_in_full(helper->helper->in, buffer->buf, buffer->len)
-		!= buffer->len)
+	if (write_in_full(helper->helper->in, buffer->buf, buffer->len) < 0)
 		die_errno("Full write to remote helper failed");
 }
 

@@ -74,7 +73,7 @@ static void write_constant(int fd, const char *str)
 {
 	if (debug)
 		fprintf(stderr, "Debug: Remote helper: -> %s", str);
-	if (write_in_full(fd, str, strlen(str)) != strlen(str))
+	if (write_in_full(fd, str, strlen(str)) < 0)
 		die_errno("Full write to remote helper failed");
 }
 

diff --git a/wrapper.c b/wrapper.c
index 36630e5d18..61aba0b5c1 100644
--- a/wrapper.c
+++ b/wrapper.c

@@ -652,7 +652,7 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...)
 void write_file_buf(const char *path, const char *buf, size_t len)
 {
 	int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
-	if (write_in_full(fd, buf, len) != len)
+	if (write_in_full(fd, buf, len) < 0)
 		die_errno(_("could not write to %s"), path);
 	if (close(fd))
 		die_errno(_("could not close %s"), path);

-- 
2.14.1.874.ge7b2e05270

The prior commit converted many sites to check the return
value of write_in_full() for negativity, rather than a
mismatch with the input length. This patch covers similar
cases, but where the return value is stored in an
intermediate variable. These should get the same treatment,
but they need to be reviewed more carefully since it would
be a bug if the return value is stored in an unsigned type
(which indeed, it is in one of the cases).

Signed-off-by: Jeff King <redacted>
---
 entry.c              | 5 +++--
 refs/files-backend.c | 2 +-
 streaming.c          | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/entry.c b/entry.c
index cb291aa88b..1c7e3c11d5 100644
--- a/entry.c
+++ b/entry.c

@@ -257,7 +257,8 @@ static int write_entry(struct cache_entry *ce,
 	char *new;
 	struct strbuf buf = STRBUF_INIT;
 	unsigned long size;
-	size_t wrote, newsize = 0;
+	ssize_t wrote;
+	size_t newsize = 0;
 	struct stat st;
 	const struct submodule *sub;
 

@@ -332,7 +333,7 @@ static int write_entry(struct cache_entry *ce,
 			fstat_done = fstat_output(fd, state, &st);
 		close(fd);
 		free(new);
-		if (wrote != size)
+		if (wrote < 0)
 			return error("unable to write file %s", path);
 		break;
 	case S_IFGITLINK:

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f8b91fff3f..489471bbcf 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -1549,7 +1549,7 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,
 
 	written = len <= maxlen ? write_in_full(fd, logrec, len) : -1;
 	free(logrec);
-	if (written != len)
+	if (written < 0)
 		return -1;
 
 	return 0;

diff --git a/streaming.c b/streaming.c
index 6f1c60f12b..5892b50bd8 100644
--- a/streaming.c
+++ b/streaming.c

@@ -540,7 +540,7 @@ int stream_blob_to_fd(int fd, const struct object_id *oid, struct stream_filter
 			kept = 0;
 		wrote = write_in_full(fd, buf, readlen);
 
-		if (wrote != readlen)
+		if (wrote < 0)
 			goto close_and_exit;
 	}
 	if (kept && (lseek(fd, kept - 1, SEEK_CUR) == (off_t) -1 ||

-- 
2.14.1.874.ge7b2e05270

As with the previous two commits, we prefer to check
write_in_full()'s return value to see if it is negative,
rather than comparing it to the input length.

These cases actually flip the logic to check for success,
making conversion a little different than in other cases. We
could of course write:

  if (write_in_full(...) >= 0)
          return 0;
  return error(...);

But our usual method of spelling write() error checks is
just "< 0". So let's flip the logic for each of these
conditionals to our usual style.

Signed-off-by: Jeff King <redacted>
---
This will have a minor textual conflict with ma/pkt-line-leakfix which
hasn't quite made it into master yet. The resolution should be
straight-forward, though.

 pkt-line.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index 7db9119573..4823d3bb9d 100644
--- a/pkt-line.c
+++ b/pkt-line.c

@@ -94,9 +94,9 @@ void packet_flush(int fd)
 int packet_flush_gently(int fd)
 {
 	packet_trace("0000", 4, 1);
-	if (write_in_full(fd, "0000", 4) == 4)
-		return 0;
-	return error("flush packet write failed");
+	if (write_in_full(fd, "0000", 4) < 0)
+		return error("flush packet write failed");
+	return 0;
 }
 
 void packet_buf_flush(struct strbuf *buf)

@@ -137,18 +137,17 @@ static int packet_write_fmt_1(int fd, int gently,
 			      const char *fmt, va_list args)
 {
 	struct strbuf buf = STRBUF_INIT;
-	ssize_t count;
 
 	format_packet(&buf, fmt, args);
-	count = write_in_full(fd, buf.buf, buf.len);
-	if (count == buf.len)
-		return 0;
-
-	if (!gently) {
-		check_pipe(errno);
-		die_errno("packet write with format failed");
+	if (write_in_full(fd, buf.buf, buf.len) < 0) {
+		if (!gently) {
+			check_pipe(errno);
+			die_errno("packet write with format failed");
+		}
+		return error("packet write with format failed");
 	}
-	return error("packet write with format failed");
+
+	return 0;
 }
 
 void packet_write_fmt(int fd, const char *fmt, ...)

@@ -183,9 +182,9 @@ static int packet_write_gently(const int fd_out, const char *buf, size_t size)
 	packet_size = size + 4;
 	set_packet_header(packet_write_buffer, packet_size);
 	memcpy(packet_write_buffer + 4, buf, size);
-	if (write_in_full(fd_out, packet_write_buffer, packet_size) == packet_size)
-		return 0;
-	return error("packet write failed");
+	if (write_in_full(fd_out, packet_write_buffer, packet_size) < 0)
+		return error("packet write failed");
+	return 0;
 }
 
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...)

-- 
2.14.1.874.ge7b2e05270

We store the return value of write_in_full() in a long,
though the return is actually an ssize_t. This probably
doesn't matter much in practice (since the buffer size is
alredy an unsigned long), but it might if the size if
between what can be represented in "long" and "unsigned
long", and if your size_t is larger than a "long" (as it is
on 64-bit Windows).

Signed-off-by: Jeff King <redacted>
---
 notes-merge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/notes-merge.c b/notes-merge.c
index b04d2f2131..597d43f65c 100644
--- a/notes-merge.c
+++ b/notes-merge.c

@@ -302,7 +302,7 @@ static void write_buf_to_worktree(const struct object_id *obj,
 	fd = xopen(path, O_WRONLY | O_EXCL | O_CREAT, 0666);
 
 	while (size > 0) {
-		long ret = write_in_full(fd, buf, size);
+		ssize_t ret = write_in_full(fd, buf, size);
 		if (ret < 0) {
 			/* Ignore epipe */
 			if (errno == EPIPE)

-- 
2.14.1.874.ge7b2e05270

The store_write_section() and store_write_pairs() functions
are basically high-level wrappers around write(). But their
return values are flipped from our usual convention, using
"1" for success and "0" for failure.

Let's flip them to follow the usual write() conventions and
update all callers. As these are local to config.c, it's
unlikely that we'd have new callers in any topics in flight
(which would be silently broken by our change). But just to
be on the safe side, let's rename them to just
write_section() and write_pairs().  That also accentuates
their relationship with write().

Signed-off-by: Jeff King <redacted>
---
 config.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/config.c b/config.c
index daf093db45..f8b7b81445 100644
--- a/config.c
+++ b/config.c

@@ -2297,10 +2297,11 @@ static int write_error(const char *filename)
 	return 4;
 }
 
-static int store_write_section(int fd, const char *key)
+static ssize_t write_section(int fd, const char *key)
 {
 	const char *dot;
-	int i, success;
+	int i;
+	ssize_t ret;
 	struct strbuf sb = STRBUF_INIT;
 
 	dot = memchr(key, '.', store.baselen);

@@ -2316,15 +2317,16 @@ static int store_write_section(int fd, const char *key)
 		strbuf_addf(&sb, "[%.*s]\n", store.baselen, key);
 	}
 
-	success = write_in_full(fd, sb.buf, sb.len) == sb.len;
+	ret = write_in_full(fd, sb.buf, sb.len);
 	strbuf_release(&sb);
 
-	return success;
+	return ret;
 }
 
-static int store_write_pair(int fd, const char *key, const char *value)
+static ssize_t write_pair(int fd, const char *key, const char *value)
 {
-	int i, success;
+	int i;
+	ssize_t ret;
 	int length = strlen(key + store.baselen + 1);
 	const char *quote = "";
 	struct strbuf sb = STRBUF_INIT;

@@ -2364,10 +2366,10 @@ static int store_write_pair(int fd, const char *key, const char *value)
 		}
 	strbuf_addf(&sb, "%s\n", quote);
 
-	success = write_in_full(fd, sb.buf, sb.len) == sb.len;
+	ret = write_in_full(fd, sb.buf, sb.len);
 	strbuf_release(&sb);
 
-	return success;
+	return ret;
 }
 
 static ssize_t find_beginning_of_line(const char *contents, size_t size,

@@ -2497,8 +2499,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 		}
 
 		store.key = (char *)key;
-		if (!store_write_section(fd, key) ||
-		    !store_write_pair(fd, key, value))
+		if (write_section(fd, key) < 0 ||
+		    write_pair(fd, key, value) < 0)
 			goto write_err_out;
 	} else {
 		struct stat st;

@@ -2620,10 +2622,10 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 		/* write the pair (value == NULL means unset) */
 		if (value != NULL) {
 			if (store.state == START) {
-				if (!store_write_section(fd, key))
+				if (write_section(fd, key) < 0)
 					goto write_err_out;
 			}
-			if (!store_write_pair(fd, key, value))
+			if (write_pair(fd, key, value) < 0)
 				goto write_err_out;
 		}
 

@@ -2816,7 +2818,7 @@ int git_config_rename_section_in_file(const char *config_filename,
 					continue;
 				}
 				store.baselen = strlen(new_name);
-				if (!store_write_section(out_fd, new_name)) {
+				if (write_section(out_fd, new_name) < 0) {
 					ret = write_error(get_lock_file_path(lock));
 					goto out;
 				}

-- 
2.14.1.874.ge7b2e05270

Hi,

Jeff King wrote:

I scoured the code base for cases of this, but it turns out
that these two in git_config_set_multivar_in_file_gently()
are the only ones. This case is actually quite interesting:
we don't have a size_t, but rather use the subtraction of
two pointers. Which you might think would be a signed
ptrdiff_t, but clearly both gcc and clang treat it as
unsigned (possibly because the conditional just above
guarantees that the result is greater than zero).

Do you have more detail about this?  I get worried when I read
something like this that sounds like a compiler bug.

C99 sayeth:

	When two pointers are subtracted, both shall point to elements
	of the same array object, or one past the last element of the
	array object; the result is the difference of the subscripts
	of the two array elements. The size of the result is
	implementation-defined, and its type (a signed integer type)
	is ptrdiff_t defined in the <stddef.h> header.

How can I reproduce the problem?

We can avoid the whole question by just checking for a
negative return value directly, as write_in_full() will
never return any value except -1 or the full count.

There's no addition to the test suite here, since you need
to convince write() to fail in order to see the problem. The
simplest reproduction recipe I came up with is to trigger
ENOSPC (this only works on Linux, obviously):

Does /dev/full make it simpler to reproduce?

Thanks,
Jonathan

On Wed, Sep 13, 2017 at 10:47:28AM -0700, Jonathan Nieder wrote:

Jeff King wrote:

quoted

I scoured the code base for cases of this, but it turns out
that these two in git_config_set_multivar_in_file_gently()
are the only ones. This case is actually quite interesting:
we don't have a size_t, but rather use the subtraction of
two pointers. Which you might think would be a signed
ptrdiff_t, but clearly both gcc and clang treat it as
unsigned (possibly because the conditional just above
guarantees that the result is greater than zero).

Do you have more detail about this?  I get worried when I read
something like this that sounds like a compiler bug.

C99 sayeth:

	When two pointers are subtracted, both shall point to elements
	of the same array object, or one past the last element of the
	array object; the result is the difference of the subscripts
	of the two array elements. The size of the result is
	implementation-defined, and its type (a signed integer type)
	is ptrdiff_t defined in the <stddef.h> header.

I'm not sure if it's a compiler bug or not. I read the bits about
ptrdiff_t, and it wasn't entirely clear to me if a pointer difference
_is_ an actual ptrdiff_t, or if it can generally be stored in one. Right
below that text it also says:

  If the result is not representable in an object of that type, the
  behavior is undefined.

That said, I might be wrong that unsigned promotion is the culprit. I
didn't look at the generated assembly. But I also can't see what else
would be causing the problem here. We're clearly returning "-1" and the
condition doesn't trigger.

How can I reproduce the problem?

I gave a recipe in the commit message, which is the best I came up with.
You could probably use a fault-injection library to convince write() to
fail. Or just tweak the source code to have write_in_full() return -1.

quoted

There's no addition to the test suite here, since you need
to convince write() to fail in order to see the problem. The
simplest reproduction recipe I came up with is to trigger
ENOSPC (this only works on Linux, obviously):

Does /dev/full make it simpler to reproduce?

I don't think so, because the write() failure is to the lockfile, which
is created with O_EXCL. So even if you could convince "config.lock" to
be the right device type, the open() would fail.

-Peff

Jeff King wrote:

We ask to write 41 bytes and make sure that the return value
is at least 41. This is the same "dangerous" pattern that
was fixed in the prior commit (wherein a negative return
value is promoted to unsigned), though it is not dangerous
here because our "41" is a constant, not an unsigned
variable.

But we should convert it anyway to avoid modeling a
dangerous construct.

If the above logic is correct, then I suspect this series does not go
far enough.  write_in_full() would be one of those APIs that is easy
to misuse and difficult to use correctly, and if so we should fix that
at the source instead of trying to teach callers not to hold it wrong.

E.g. what would you think of

 1. Introduce a write_fully (sorry, I am bad at names) function
    that returns 0 on success and a coccinelle semantic patch in
    contrib/coccinelle to migrate callers in "make coccicheck":

@@
expression E;
expression F;
expression G;
@@
-write_in_full(E, F, G) < G
+write_fully(E, F, G)

 2. Run "make coccicheck" and apply the result.
 3. Remove the write_in_full function.

Does read_in_full need a similar treatment?

Thanks,
Jonathan

Jeff King wrote:

On Wed, Sep 13, 2017 at 10:47:28AM -0700, Jonathan Nieder wrote:

quoted

Jeff King wrote:

quoted

quoted

I scoured the code base for cases of this, but it turns out
that these two in git_config_set_multivar_in_file_gently()
are the only ones. This case is actually quite interesting:
we don't have a size_t, but rather use the subtraction of
two pointers. Which you might think would be a signed
ptrdiff_t, but clearly both gcc and clang treat it as
unsigned (possibly because the conditional just above
guarantees that the result is greater than zero).

Do you have more detail about this?  I get worried when I read
something like this that sounds like a compiler bug.

C99 sayeth:

	When two pointers are subtracted, both shall point to elements
	of the same array object, or one past the last element of the
	array object; the result is the difference of the subscripts
	of the two array elements. The size of the result is
	implementation-defined, and its type (a signed integer type)
	is ptrdiff_t defined in the <stddef.h> header.

I'm not sure if it's a compiler bug or not. I read the bits about
ptrdiff_t, and it wasn't entirely clear to me if a pointer difference
_is_ an actual ptrdiff_t, or if it can generally be stored in one. Right
below that text it also says:

  If the result is not representable in an object of that type, the
  behavior is undefined.

I can confidentally say the intent in C99 in that passage is to
describe the type of the expression, not just the type of a variable
that can hold it.

That said, I might be wrong that unsigned promotion is the culprit. I
didn't look at the generated assembly. But I also can't see what else
would be causing the problem here. We're clearly returning "-1" and the
condition doesn't trigger.

quoted

How can I reproduce the problem?

I gave a recipe in the commit message, which is the best I came up with.
You could probably use a fault-injection library to convince write() to
fail. Or just tweak the source code to have write_in_full() return -1.

I wonder if a new test helper in t/helper/ would be able to do it (since
then it could e.g. control the filename that write_in_full writes to).

quoted

quoted

There's no addition to the test suite here, since you need
to convince write() to fail in order to see the problem. The
simplest reproduction recipe I came up with is to trigger
ENOSPC (this only works on Linux, obviously):

Does /dev/full make it simpler to reproduce?

I don't think so, because the write() failure is to the lockfile, which
is created with O_EXCL. So even if you could convince "config.lock" to
be the right device type, the open() would fail.

Hm, you're convincing me that it would indeed be worth hooking into a
fault injection framework (that e.g. uses LD_PRELOAD), but that's a
topic for another day.

Thanks,
Jonathan

On Wed, Sep 13, 2017 at 10:53:57AM -0700, Jonathan Nieder wrote:

Jeff King wrote:

quoted

We ask to write 41 bytes and make sure that the return value
is at least 41. This is the same "dangerous" pattern that
was fixed in the prior commit (wherein a negative return
value is promoted to unsigned), though it is not dangerous
here because our "41" is a constant, not an unsigned
variable.

But we should convert it anyway to avoid modeling a
dangerous construct.

If the above logic is correct, then I suspect this series does not go
far enough.  write_in_full() would be one of those APIs that is easy
to misuse and difficult to use correctly, and if so we should fix that
at the source instead of trying to teach callers not to hold it wrong.

Yes, this series is just removing bad examples. It doesn't do anything
to make write_in_full() less potentially dangerous.

On the other hand, it's no more or less dangerous than write(), which
has the same return-value semantics.

E.g. what would you think of

 1. Introduce a write_fully (sorry, I am bad at names) function
    that returns 0 on success and a coccinelle semantic patch in
    contrib/coccinelle to migrate callers in "make coccicheck":

Yes, I considered that, though some callers really do care about
assigning the number of bytes written. The fact that write() has the
same problem, plus the fact that there were only 2 buggy instances
across the code base made me think there's not a huge gain to that extra
step.

@@
expression E;
expression F;
expression G;
@@
-write_in_full(E, F, G) < G
+write_fully(E, F, G)

 2. Run "make coccicheck" and apply the result.
 3. Remove the write_in_full function.

There's a step between those where you have to update all of the
write_in_full() callers that store the result. Some of them would be
trivial conversions, but some of them actually care about the length
E.g., the one in imap-send.c, which is the only one I didn't convert
away from "!= len" because it's half of an #ifdef with SSL_write()
(which uses an "int" as the return value!).

Does read_in_full need a similar treatment?

It might actually return fewer than the requested number of bytes, so it
can't just use "< 0" in the same way (nor be adapted to return 0 on
success).  But I think it's still a bug to do:

  char buf[20];
  size_t len = sizeof(buf);
  if (read_in_full(fd, buf, len) < len)
          die(...);

since that will promote the -1 to a size_t. So it's probably worth
auditing.

-Peff

On Wed, Sep 13, 2017 at 10:59:30AM -0700, Jonathan Nieder wrote:

I can confidentally say the intent in C99 in that passage is to
describe the type of the expression, not just the type of a variable
that can hold it.

Doh. The problem is that I'm a moron. The copy_begin and copy_end values
are _not_ pointers, they're size_t. That's why we have to use:

  contents + copy_begin

as the buffer. So there is no ptrdiff_t involved at all, just a size_t.

-Peff

On Wed, Sep 13, 2017 at 01:11:04PM -0400, Jeff King wrote:

I scoured the code base for cases of this, but it turns out
that these two in git_config_set_multivar_in_file_gently()
are the only ones. This case is actually quite interesting:
we don't have a size_t, but rather use the subtraction of
two pointers. Which you might think would be a signed
ptrdiff_t, but clearly both gcc and clang treat it as
unsigned (possibly because the conditional just above
guarantees that the result is greater than zero).

Sorry, this is totally wrong. As Jonathan pointed out (and as I was
puzzled over here), the result of a pointer subtraction _is_ a signed
type, and works fine.

What I missed is that copy_begin and copy_end here are actually size_t
variables, not the pointers. Sorry for the confusion, and here's an
updated version of the patch with this paragraph amended (the patch
itself is identical):

-- >8 --
Subject: [PATCH] config: avoid "write_in_full(fd, buf, len) < len" pattern

The return type of write_in_full() is a signed ssize_t,
because we may return "-1" on failure (even if we succeeded
in writing some bytes). But "len" itself is may be an
unsigned type (the function takes a size_t, but of course we
may have something else in the calling function). So while
it seems like:

  if (write_in_full(fd, buf, len) < len)
	die_errno("write error");

would trigger on error, it won't if "len" is unsigned.  The
compiler sees a signed/unsigned comparison and promotes the
signed value, resulting in (size_t)-1, the highest possible
size_t (or again, whatever type the caller has). This cannot
possibly be smaller than "len", and so the conditional can
never trigger.

I scoured the code base for cases of this, but it turns out
that these two in git_config_set_multivar_in_file_gently()
are the only ones. Here our "len" is the difference between
two size_t variables, making the result an unsigned size_t.
We can fix this by just checking for a negative return value
directly, as write_in_full() will never return any value
except -1 or the full count.

There's no addition to the test suite here, since you need
to convince write() to fail in order to see the problem. The
simplest reproduction recipe I came up with is to trigger
ENOSPC:

  # make a limited-size filesystem
  dd if=/dev/zero of=small.disk bs=1M count=1
  mke2fs small.disk
  mkdir mnt
  sudo mount -o loop small.disk mnt
  cd mnt
  sudo chown $USER:$USER .

  # make a config file with some content
  git config --file=config one.key value
  git config --file=config two.key value

  # now fill up the disk
  dd if=/dev/zero of=fill

  # and try to delete a key, which requires copying the rest
  # of the file to config.lock, and will fail on write()
  git config --file=config --unset two.key

That final command should (and does after this patch)
produce an error message due to the failed write, and leave
the file intact. Instead, it silently ignores the failure
and renames config.lock into place, leaving you with a
totally empty config file!

Reported-by: demerphq <redacted>
Signed-off-by: Jeff King <redacted>
---
 config.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/config.c b/config.c
index d0d8ce823a..eee4ac0355 100644
--- a/config.c
+++ b/config.c

@@ -2608,8 +2608,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 			/* write the first part of the config */
 			if (copy_end > copy_begin) {
 				if (write_in_full(fd, contents + copy_begin,
-						  copy_end - copy_begin) <
-				    copy_end - copy_begin)
+						  copy_end - copy_begin) < 0)
 					goto write_err_out;
 				if (new_line &&
 				    write_str_in_full(fd, "\n") != 1)

@@ -2631,8 +2630,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 		/* write the rest of the config */
 		if (copy_begin < contents_sz)
 			if (write_in_full(fd, contents + copy_begin,
-					  contents_sz - copy_begin) <
-			    contents_sz - copy_begin)
+					  contents_sz - copy_begin) < 0)
 				goto write_err_out;
 
 		munmap(contents, contents_sz);

-- 
2.14.1.874.ge7b2e05270

Jeff King wrote:

What I missed is that copy_begin and copy_end here are actually size_t
variables, not the pointers. Sorry for the confusion, and here's an
updated version of the patch with this paragraph amended (the patch
itself is identical):

Subtle.  The world makes more sense now.  Thanks for figuring it out.

-- >8 --
Subject: [PATCH] config: avoid "write_in_full(fd, buf, len) < len" pattern

The return type of write_in_full() is a signed ssize_t,
because we may return "-1" on failure (even if we succeeded
in writing some bytes). But "len" itself is may be an
unsigned type (the function takes a size_t, but of course we
may have something else in the calling function). So while
it seems like:

  if (write_in_full(fd, buf, len) < len)
	die_errno("write error");

would trigger on error, it won't if "len" is unsigned.  The
compiler sees a signed/unsigned comparison and promotes the
signed value, resulting in (size_t)-1, the highest possible
size_t (or again, whatever type the caller has). This cannot
possibly be smaller than "len", and so the conditional can
never trigger.

I scoured the code base for cases of this, but it turns out
that these two in git_config_set_multivar_in_file_gently()
are the only ones. Here our "len" is the difference between
two size_t variables, making the result an unsigned size_t.
We can fix this by just checking for a negative return value
directly, as write_in_full() will never return any value
except -1 or the full count.

[...]

Reported-by: demerphq <redacted>
Signed-off-by: Jeff King <redacted>
---

For what it's worth,
Reviewed-by: Jonathan Nieder <redacted>

Thank you.

Compilers' signed/unsigned comparison warning can be noisy, but I'm
starting to feel it's worth the suppression noise to turn it on when
DEVELOPER=1 anyway.  What do you think?  Is there a way to turn it on
selectively for certain functions on the LHS (like read() and write()
style functions)?

Sincerely,
Jonathan

On Wed, Sep 13, 2017 at 02:02:32PM -0400, Jeff King wrote:

quoted

Does read_in_full need a similar treatment?

It might actually return fewer than the requested number of bytes, so it
can't just use "< 0" in the same way (nor be adapted to return 0 on
success).  But I think it's still a bug to do:

  char buf[20];
  size_t len = sizeof(buf);
  if (read_in_full(fd, buf, len) < len)
          die(...);

since that will promote the -1 to a size_t. So it's probably worth
auditing.

I looked it over and found one potentially buggy case. In
read-pack_header(), we do:

          if (read_in_full(fd, header, sizeof(*header)) < sizeof(*header))
                  /* "eof before pack header was fully read" */
                  return PH_ERROR_EOF;

which will treat a read error as a silent success. I don't think it's
harmful in practice because the next line checks that the header
matches the PACK signature (which yes, means we're reading uninitialized
bytes, but the chances are 1 in 2^32 that they match the signature.
Unless the buffer had an old PACK signature in it, of course ;) ).

There's one other harmless "< len" check. There are a bunch of "!="
checks which are OK as-is. Converting them to something equally short is
hard, though we could introduce a helper similar to your write_fully(),
like:

  int read_exactly(int fd, char *buf, size_t len)
  {
	ssize_t ret = read_in_full(fd, buf, len);
	if (ret < 0)
		return -1; /* real error */
	else if (ret < len)
		return -1; /* early eof */
	return 0;
  }

But the trouble is that a _good_ caller actually handles those cases
separately to provide better error messages (by doing that same
if-cascade themselves). If those if's were turned into error() or die()
calls, then we'd actually be improving the callsites. But of course not
all of them actually print an error or die. And when they do, they
usually say something specific about _what_ they were trying to read.

So it may not be worth trying to do a mass-cleanup in the same way here.

-Peff

On Wed, Sep 13, 2017 at 01:08:07PM -0400, Jeff King wrote:

Here's the fix, along with some related cleanups. The actual bugfix is
in the first patch, which I think should go to "maint". The rest are not
as important, so could go to master (but would also be fine for maint,
as there should be no user-visible changes).

  [1/7]: config: avoid "write_in_full(fd, buf, len) < len" pattern
  [2/7]: get-tar-commit-id: check write_in_full() return against 0
  [3/7]: avoid "write_in_full(fd, buf, len) != len" pattern
  [4/7]: convert less-trivial versions of "write_in_full() != len"
  [5/7]: pkt-line: check write_in_full() errors against "< 0"
  [6/7]: notes-merge: use ssize_t for write_in_full() return value
  [7/7]: config: flip return value of store_write_*()

Jonathan pointed out that read_in_full() can suffer from the same issue,
and there is one buggy caller.

A mass cleanup is a bit harder, as described in

  https://public-inbox.org/git/20170913183700.amtrquhg66hjrbsp@sigill.intra.peff.net/

so I punted on it for now.

-- >8 --
Subject: [PATCH] read_pack_header: handle signed/unsigned comparison in read
 result

The result of read_in_full() may be -1 if we saw an error.
But in comparing it to a sizeof() result, that "-1" will be
promoted to size_t. In fact, the largest possible size_t
which is much bigger than our struct size. This means that
our "< sizeof(header)" error check won't trigger.

In practice, we'd go on to read uninitialized memory and
compare it to the PACK signature, which is likely to fail.
But we shouldn't get there.

We can fix this by making a direct "!=" comparison to the
requested size, rather than "<". This means that errors get
lumped in with short reads, but that's sufficient for our
purposes here. There's no PH_ERROR tp represent our case.
And anyway, this function reads from pipes and network
sockets. A network error may racily appear as EOF to us
anyway if there's data left in the socket buffers.

Signed-off-by: Jeff King <redacted>
---
 sha1_file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index 5f71bbac3e..c1c6e9021d 100644
--- a/sha1_file.c
+++ b/sha1_file.c

@@ -1850,7 +1850,7 @@ int index_path(struct object_id *oid, const char *path, struct stat *st, unsigne
 
 int read_pack_header(int fd, struct pack_header *header)
 {
-	if (read_in_full(fd, header, sizeof(*header)) < sizeof(*header))
+	if (read_in_full(fd, header, sizeof(*header)) != sizeof(*header))
 		/* "eof before pack header was fully read" */
 		return PH_ERROR_EOF;
 

-- 
2.14.1.874.ge7b2e05270

On Wed, Sep 13, 2017 at 11:24:31AM -0700, Jonathan Nieder wrote:

For what it's worth,
Reviewed-by: Jonathan Nieder <redacted>

Thanks, and thank you for questioning the ptrdiff_t bits that didn't
make sense. I _thought_ they felt like nonsense, but couldn't quite
puzzle it out.

Compilers' signed/unsigned comparison warning can be noisy, but I'm
starting to feel it's worth the suppression noise to turn it on when
DEVELOPER=1 anyway.  What do you think?  Is there a way to turn it on
selectively for certain functions on the LHS (like read() and write()
style functions)?

Obviously we couldn't turn them on for -Werror at this point. Let me see
how bad "-Wsign-compare -Wno-error=sign-compare" is.

  $ make 2>&1 | grep -c warning:
  1391

Ouch. I'm afraid that's probably not going to be very helpful. Even if I
were introducing a new problem, I'm not likely to see it amidst the mass
of existing complaints.

AFAIK there's no way to turn it on for specific functions, but I'm far
from a gcc-warning guru. Even if you could, though, the error may be far
from the function (e.g., if we store the result in an ssize_t and then
compare that with a size_t).

Interestingly in the write_in_full() case the code was actually
unreachable! But I don't think the compiler is smart enough to know
that, because it would have to realize that write_in_full() can only
return either -1 or the original value (and not a positive value in
between). Coverity _might_ be smart enough, but figuring that out does
take some loop analysis (including the assumption that write() never
returns more bytes than we asked it to write).

-Peff

Jeff King wrote:

Subject: [PATCH] read_pack_header: handle signed/unsigned comparison in read  result

The result of read_in_full() may be -1 if we saw an error.
But in comparing it to a sizeof() result, that "-1" will be
promoted to size_t. In fact, the largest possible size_t
which is much bigger than our struct size. This means that
our "< sizeof(header)" error check won't trigger.

In practice, we'd go on to read uninitialized memory and
compare it to the PACK signature, which is likely to fail.
But we shouldn't get there.

We can fix this by making a direct "!=" comparison to the
requested size, rather than "<". This means that errors get
lumped in with short reads, but that's sufficient for our
purposes here. There's no PH_ERROR tp represent our case.
And anyway, this function reads from pipes and network
sockets. A network error may racily appear as EOF to us
anyway if there's data left in the socket buffers.

Signed-off-by: Jeff King <redacted>
---
 sha1_file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

This patch 8 (but not patches 2-7) is
Reviewed-by: Jonathan Nieder <redacted>

Thanks.

An alternative would be to use something like

	< (int) sizeof(*header)

to force it to be signed, but I think this is clearer.

Using the following semantic patch, I find this and the example from
patch 1 and no others:

  @@
  expression fd;
  expression buf;
  expression len;
  size_t rhs;
  @@
  -read_in_full(fd, buf, len) < rhs
  +ERROR()

  @@
  expression fd;
  expression buf;
  expression len;
  size_t rhs;
  @@
  -write_in_full(fd, buf, len) < rhs
  +ERROR()

Thanks,
Jonathan

Jeff King wrote:

On Wed, Sep 13, 2017 at 11:24:31AM -0700, Jonathan Nieder wrote:

quoted

Compilers' signed/unsigned comparison warning can be noisy, but I'm
starting to feel it's worth the suppression noise to turn it on when
DEVELOPER=1 anyway.  What do you think?  Is there a way to turn it on
selectively for certain functions on the LHS (like read() and write()
style functions)?

Obviously we couldn't turn them on for -Werror at this point. Let me see
how bad "-Wsign-compare -Wno-error=sign-compare" is.

  $ make 2>&1 | grep -c warning:
  1391

Ouch. I'm afraid that's probably not going to be very helpful. Even if I
were introducing a new problem, I'm not likely to see it amidst the mass
of existing complaints.

Yup.

Something like http://coccinelle.lip6.fr/rules/find_unsigned.html (but
not that one precisely, since it's not smart enough to follow typedefs
like size_t!) can do this for specific functions.  Then "make
coccicheck" would report new problematic usages.  Let me know if this
seems worth pursuing, and I'll send a patch based on the spatch I used
to review your patch 8.

Using coccinelle for this is a kind of oversized hammer.  I wonder if
e.g. sparse would be able to do a better job.

AFAIK there's no way to turn it on for specific functions, but I'm far
from a gcc-warning guru. Even if you could, though, the error may be far
from the function (e.g., if we store the result in an ssize_t and then
compare that with a size_t).

If we have a check that catches the obvious cases then I'm already
pretty happy.

Though checking ssize_t vs size_t comparisons in general might also
not be a bad idea, since it would be narrower than the general
-Wsign-compare check.

Thanks,
Jonathan

Jeff King wrote:

AFAIK there's no way to turn it on for specific functions, but I'm far
from a gcc-warning guru. Even if you could, though, the error may be far
from the function (e.g., if we store the result in an ssize_t and then
compare that with a size_t).

It turns out that yes, we have two of those.  Both handle errors
separately already, so they should be safe.

While investigating the second, I noticed that read_in_full can
overflow its return value.  malloc doesn't typically allow allocating
a buffer with size greater than SSIZE_MAX so this should be safe, but
it would be confusing to static analyzers.

Combining these observations yields the following (just for
illustration):

diff --git i/bulk-checkin.c w/bulk-checkin.c
index 9a1f6c49ab..f8e3060041 100644
--- i/bulk-checkin.c
+++ w/bulk-checkin.c

@@ -114,7 +114,7 @@ static int stream_to_pack(struct bulk_checkin_state *state,
 		unsigned char ibuf[16384];
 
 		if (size && !s.avail_in) {
-			ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
+			size_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
 			if (read_in_full(fd, ibuf, rsize) != rsize)
 				die("failed to read %d bytes from '%s'",
 				    (int)rsize, path);

diff --git i/combine-diff.c w/combine-diff.c
index 9e163d5ada..e966d4f393 100644
--- i/combine-diff.c
+++ w/combine-diff.c

@@ -1026,7 +1026,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			result_size = fill_textconv(textconv, df, &result);
 			free_filespec(df);
 		} else if (0 <= (fd = open(elem->path, O_RDONLY))) {
-			size_t len = xsize_t(st.st_size);
+			ssize_t len = xssize_t(st.st_size);
 			ssize_t done;
 			int is_file, i;
 

@@ -1040,6 +1040,8 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			if (!is_file)
 				elem->mode = canon_mode(S_IFLNK);
 
+			if (len > ULONG_MAX)
+				die("cannot handle files this big");
 			result_size = len;
 			result = xmallocz(len);
 

diff --git i/git-compat-util.h w/git-compat-util.h
index 6678b488cc..20fea81589 100644
--- i/git-compat-util.h
+++ w/git-compat-util.h

@@ -903,6 +903,13 @@ static inline size_t xsize_t(off_t len)
 	return (size_t)len;
 }
 
+static inline ssize_t xssize_t(off_t len)
+{
+	if (len > SSIZE_MAX)
+		die("cannot handle files this big");
+	return (ssize_t)len;
+}
+
 __attribute__((format (printf, 3, 4)))
 extern int xsnprintf(char *dst, size_t max, const char *fmt, ...);
 

diff --git i/wrapper.c w/wrapper.c
index 36630e5d18..2b52b7367d 100644
--- i/wrapper.c
+++ w/wrapper.c

@@ -314,6 +314,9 @@ ssize_t read_in_full(int fd, void *buf, size_t count)
 	char *p = buf;
 	ssize_t total = 0;
 
+	if (count > SSIZE_MAX)
+		BUG("read_in_full called with absurdly high count %"PRIuMAX,
+		    (uintmax_t) count);
 	while (count > 0) {
 		ssize_t loaded = xread(fd, p, count);
 		if (loaded < 0)

Hi,

Jeff King wrote:

We ask to write 41 bytes and make sure that the return value
is at least 41. This is the same "dangerous" pattern that
was fixed in the prior commit (wherein a negative return
value is promoted to unsigned), though it is not dangerous
here because our "41" is a constant, not an unsigned
variable.

But we should convert it anyway to avoid modeling a
dangerous construct.

Signed-off-by: Jeff King <redacted>
---
 builtin/get-tar-commit-id.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

I kind of disagree with calling this dangerous (and I think that is
what you alluded to above by putting it in quotes), but I like the
postimage more than the preimage.

The variable 'n' could be eliminated to simplify this further.  I
realize that would go against the spirit of this patch, but (1) it's
on-topic for the patch, since it is another ssize_t vs constant
comparison and (2) as mentioned elsewhere in this thread, it's a very
common idiom with read_in_full.  If we want to eliminate it then we
could introduce a separate helper to distinguish between
read_this_much_i_mean_it and read_this_much_or_to_eof.

Anyway, I think the below is an improvement.

With or without this tweak,
Reviewed-by: Jonathan Nieder <redacted>

Thanks.

diff --git i/builtin/get-tar-commit-id.c w/builtin/get-tar-commit-id.c
index 6d9a79f9b3..03ef7c5ba4 100644
--- i/builtin/get-tar-commit-id.c
+++ w/builtin/get-tar-commit-id.c

@@ -20,13 +20,11 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix)
 	struct ustar_header *header = (struct ustar_header *)buffer;
 	char *content = buffer + RECORDSIZE;
 	const char *comment;
-	ssize_t n;
 
 	if (argc != 1)
 		usage(builtin_get_tar_commit_id_usage);
 
-	n = read_in_full(0, buffer, HEADERSIZE);
-	if (n < HEADERSIZE)
+	if (read_in_full(0, buffer, HEADERSIZE) < HEADERSIZE)
 		die("git get-tar-commit-id: read error");
 	if (header->typeflag[0] != 'g')
 		return 1;

Jeff King wrote:

The return value of write_in_full() is either "-1", or the
requested number of bytes[1]. If we make a partial write
before seeing an error, we still return -1, not a partial
value. This goes back to f6aa66cb95 (write_in_full: really
write in full or return error on disk full., 2007-01-11).

So checking anything except "was the return value negative"
is pointless. And there are a couple of reasons not to do
so:

  1. It can do a funny signed/unsigned comparison. If your

[...]

  2. Checking for a negative value is shorter to type,
     especially when the length is an expression.

  3. Linus says so. In d34cf19b89 (Clean up write_in_full()
     users, 2007-01-11), right after the write_in_full()
     semantics were changed, he wrote:

       I really wish every "write_in_full()" user would just
       check against "<0" now, but this fixes the nasty and
       stupid ones.

Ok, you convinced me.

Should we add a comment to cache.h as well encouraging this?

[...]

[1] A careful reader may notice there is one way that
    write_in_full() can return a different value. If we ask
    write() to write N bytes and get a return value that is
    _larger_ than N, we could return a larger total. But
    besides the fact that this would imply a totally broken
    version of write(), it would already invoke undefined
    behavior. Our internal remaining counter is an unsigned
    size_t, which means that subtracting too many byte will
    wrap it around to a very large number. So we'll instantly
    begin reading off the end of the buffer, trying to write
    gigabytes (or petabytes) of data.

This footnote just leaves me more confused, since as you mention,
write() never would return a value greater than N.  Are you saying we
need to defend against a broken platform where that isn't true?

Signed-off-by: Jeff King <redacted>
---
 builtin/receive-pack.c | 2 +-
 builtin/rerere.c       | 2 +-
 builtin/unpack-file.c  | 2 +-
 config.c               | 4 ++--
 diff.c                 | 2 +-
 fast-import.c          | 2 +-
 http-backend.c         | 4 ++--
 ll-merge.c             | 2 +-
 read-cache.c           | 6 +++---
 refs.c                 | 2 +-
 refs/files-backend.c   | 8 ++++----
 rerere.c               | 2 +-
 shallow.c              | 6 +++---
 t/helper/test-delta.c  | 2 +-
 transport-helper.c     | 5 ++---
 wrapper.c              | 2 +-
 16 files changed, 26 insertions(+), 27 deletions(-)

All of these look correctly done.

Reviewed-by: Jonathan Nieder <redacted>

Jeff King wrote:

The prior commit converted many sites to check the return
value of write_in_full() for negativity, rather than a
mismatch with the input length. This patch covers similar
cases, but where the return value is stored in an
intermediate variable. These should get the same treatment,
but they need to be reviewed more carefully since it would
be a bug if the return value is stored in an unsigned type
(which indeed, it is in one of the cases).

Signed-off-by: Jeff King <redacted>
---
 entry.c              | 5 +++--
 refs/files-backend.c | 2 +-
 streaming.c          | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Jonathan Nieder <redacted>

Jeff King wrote:

As with the previous two commits, we prefer to check
write_in_full()'s return value to see if it is negative,
rather than comparing it to the input length.

These cases actually flip the logic to check for success,
making conversion a little different than in other cases. We
could of course write:

  if (write_in_full(...) >= 0)
          return 0;
 return error(...);

But our usual method of spelling write() error checks is
just "< 0". So let's flip the logic for each of these
conditionals to our usual style.

Signed-off-by: Jeff King <redacted>
---
 pkt-line.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

Reviewed-by: Jonathan Nieder <redacted>

Jeff King wrote:

We store the return value of write_in_full() in a long,
though the return is actually an ssize_t. This probably
doesn't matter much in practice (since the buffer size is
alredy an unsigned long), but it might if the size if
between what can be represented in "long" and "unsigned
long", and if your size_t is larger than a "long" (as it is
on 64-bit Windows).

Signed-off-by: Jeff King <redacted>
---
 notes-merge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Yikes.  Good catch.

With or without the tweak below,
Reviewed-by: Jonathan Nieder <redacted>

These three lines are dead code.  How about the following, e.g. for
squashing in?

diff --git i/notes-merge.c w/notes-merge.c
index 597d43f65c..4352c34a6e 100644
--- i/notes-merge.c
+++ w/notes-merge.c

@@ -308,8 +308,6 @@ static void write_buf_to_worktree(const struct object_id *obj,
 			if (errno == EPIPE)
 				break;
 			die_errno("notes-merge");
-		} else if (!ret) {
-			die("notes-merge: disk full?");
 		}
 		size -= ret;
 		buf += ret;

Hi,

Jeff King wrote:

The store_write_section() and store_write_pairs() functions
are basically high-level wrappers around write(). But their
return values are flipped from our usual convention, using
"1" for success and "0" for failure.

Let's flip them to follow the usual write() conventions and
update all callers. As these are local to config.c, it's
unlikely that we'd have new callers in any topics in flight
(which would be silently broken by our change). But just to
be on the safe side, let's rename them to just
write_section() and write_pairs().  That also accentuates
their relationship with write().

Signed-off-by: Jeff King <redacted>

The caller only cares if it succeeded, right?  Could this return
the customary 0 vs -1 instead of the number of bytes written?

That would look like the following (as a patch to squash in):

With or without that tweak,
Reviewed-by: Jonathan Nieder <redacted>

diff --git i/config.c w/config.c
index 272a32aac0..8f92d452bf 100644
--- i/config.c
+++ w/config.c

@@ -2297,11 +2297,10 @@ static int write_error(const char *filename)
 	return 4;
 }
 
-static ssize_t write_section(int fd, const char *key)
+static int write_section(int fd, const char *key)
 {
 	const char *dot;
-	int i;
-	ssize_t ret;
+	int i, ret;
 	struct strbuf sb = STRBUF_INIT;
 
 	dot = memchr(key, '.', store.baselen);

@@ -2317,16 +2316,15 @@ static ssize_t write_section(int fd, const char *key)
 		strbuf_addf(&sb, "[%.*s]\n", store.baselen, key);
 	}
 
-	ret = write_in_full(fd, sb.buf, sb.len);
+	ret = write_in_full(fd, sb.buf, sb.len) < 0 ? -1 : 0;
 	strbuf_release(&sb);
 
 	return ret;
 }
 
-static ssize_t write_pair(int fd, const char *key, const char *value)
+static int write_pair(int fd, const char *key, const char *value)
 {
-	int i;
-	ssize_t ret;
+	int i, ret;
 	int length = strlen(key + store.baselen + 1);
 	const char *quote = "";
 	struct strbuf sb = STRBUF_INIT;

@@ -2366,7 +2364,7 @@ static ssize_t write_pair(int fd, const char *key, const char *value)
 		}
 	strbuf_addf(&sb, "%s\n", quote);
 
-	ret = write_in_full(fd, sb.buf, sb.len);
+	ret = write_in_full(fd, sb.buf, sb.len) < 0 ? -1 : 0;
 	strbuf_release(&sb);
 
 	return ret;

@@ -2499,8 +2497,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 		}
 
 		store.key = (char *)key;
-		if (write_section(fd, key) < 0 ||
-		    write_pair(fd, key, value) < 0)
+		if (write_section(fd, key) || write_pair(fd, key, value))
 			goto write_err_out;
 	} else {
 		struct stat st;

@@ -2623,10 +2620,10 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 		/* write the pair (value == NULL means unset) */
 		if (value != NULL) {
 			if (store.state == START) {
-				if (write_section(fd, key) < 0)
+				if (write_section(fd, key))
 					goto write_err_out;
 			}
-			if (write_pair(fd, key, value) < 0)
+			if (write_pair(fd, key, value))
 				goto write_err_out;
 		}
 

@@ -2820,7 +2817,7 @@ int git_config_rename_section_in_file(const char *config_filename,
 					continue;
 				}
 				store.baselen = strlen(new_name);
-				if (write_section(out_fd, new_name) < 0) {
+				if (write_section(out_fd, new_name)) {
 					ret = write_error(get_lock_file_path(lock));
 					goto out;
 				}

On 13/09/17 19:58, Jeff King wrote:

On Wed, Sep 13, 2017 at 11:24:31AM -0700, Jonathan Nieder wrote:

quoted

For what it's worth,
Reviewed-by: Jonathan Nieder <redacted>

Thanks, and thank you for questioning the ptrdiff_t bits that didn't
make sense. I _thought_ they felt like nonsense, but couldn't quite
puzzle it out.

quoted

Compilers' signed/unsigned comparison warning can be noisy, but I'm
starting to feel it's worth the suppression noise to turn it on when
DEVELOPER=1 anyway.  What do you think?  Is there a way to turn it on
selectively for certain functions on the LHS (like read() and write()
style functions)?

Obviously we couldn't turn them on for -Werror at this point. Let me see
how bad "-Wsign-compare -Wno-error=sign-compare" is.

  $ make 2>&1 | grep -c warning:
  1391

Ouch. I'm afraid that's probably not going to be very helpful. Even if I
were introducing a new problem, I'm not likely to see it amidst the mass
of existing complaints.

Hmm, about three or four years ago, I spent two or three evenings
getting git to compile -Wextra clean. I remember the signed/unsigned
issue was the cause of a large portion of the warnings issued by
the compiler. I was surprised that it took such a relatively short
time to do. However, it affected quite a large portion of the code, so
I didn't think Junio would even consider applying it. Also, I only used
gcc and was anticipating having additional warnings on clang (but I
didn't get around to trying).

Maybe I should give it another go. :-D

ATB,
Ramsay Jones

On 13/09/17 23:43, Ramsay Jones wrote:

On 13/09/17 19:58, Jeff King wrote:

quoted

On Wed, Sep 13, 2017 at 11:24:31AM -0700, Jonathan Nieder wrote:

quoted

For what it's worth,
Reviewed-by: Jonathan Nieder <redacted>

Thanks, and thank you for questioning the ptrdiff_t bits that didn't
make sense. I _thought_ they felt like nonsense, but couldn't quite
puzzle it out.

quoted

Compilers' signed/unsigned comparison warning can be noisy, but I'm
starting to feel it's worth the suppression noise to turn it on when
DEVELOPER=1 anyway.  What do you think?  Is there a way to turn it on
selectively for certain functions on the LHS (like read() and write()
style functions)?

Obviously we couldn't turn them on for -Werror at this point. Let me see
how bad "-Wsign-compare -Wno-error=sign-compare" is.

  $ make 2>&1 | grep -c warning:
  1391

Ouch. I'm afraid that's probably not going to be very helpful. Even if I
were introducing a new problem, I'm not likely to see it amidst the mass
of existing complaints.

Hmm, about three or four years ago, I spent two or three evenings
getting git to compile -Wextra clean. I remember the signed/unsigned
issue was the cause of a large portion of the warnings issued by
the compiler. I was surprised that it took such a relatively short
time to do. However, it affected quite a large portion of the code, so
I didn't think Junio would even consider applying it. Also, I only used
gcc and was anticipating having additional warnings on clang (but I
didn't get around to trying).

Maybe I should give it another go. :-D

For example, I remember the patch given below reduced the number
of warnings quite a bit (because it's an inline function in a
header file).

I just tried it again tonight; the current master branch has 3532
warnings when compiled with -Wextra, 1409 of which are -Wsign-compare
warnings. After applying the patch below, those numbers are reduced
by 344 to 3188/1065.

Still quite a bit to go ... ;-)

ATB,
Ramsay Jones

-- >8 --
Subject: [PATCH] git-compat-util: avoid -Wsign-compare warnings from xsize_t()

Signed-off-by: Ramsay Jones <redacted>
---
 git-compat-util.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 6678b488c..2f3cf0883 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -898,9 +898,11 @@ static inline char *xstrdup_or_null(const char *str)
 
 static inline size_t xsize_t(off_t len)
 {
-	if (len > (size_t) len)
+	size_t size = (size_t) len;
+
+	if (len != (off_t) size)
 		die("Cannot handle files this big");
-	return (size_t)len;
+	return size;
 }
 
 __attribute__((format (printf, 3, 4)))

-- 
2.14.0

On Thu, Sep 14, 2017 at 12:31:38AM +0100, Ramsay Jones wrote:

quoted

Hmm, about three or four years ago, I spent two or three evenings
getting git to compile -Wextra clean. I remember the signed/unsigned
issue was the cause of a large portion of the warnings issued by
the compiler. I was surprised that it took such a relatively short
time to do. However, it affected quite a large portion of the code, so
I didn't think Junio would even consider applying it. Also, I only used
gcc and was anticipating having additional warnings on clang (but I
didn't get around to trying).

Maybe I should give it another go. :-D

For example, I remember the patch given below reduced the number
of warnings quite a bit (because it's an inline function in a
header file).

I just tried it again tonight; the current master branch has 3532
warnings when compiled with -Wextra, 1409 of which are -Wsign-compare
warnings. After applying the patch below, those numbers are reduced
by 344 to 3188/1065.

I'd love it if we were clean on -Wextra. My big question is many
contortions we'd have to go through in the code. I don't mind at all if
we're actually cleaning as we go (e.g., using types of the correct
signedness, or preventing possible funny interactions). I'm just worried
it will turn into a bunch of noisy casts.

The patch you showed seems like an improvement to the code, but I don't
know if that would be the case for all of them. :)

-Peff

On Wed, Sep 13, 2017 at 02:09:27PM -0700, Jonathan Nieder wrote:

quoted

We ask to write 41 bytes and make sure that the return value
is at least 41. This is the same "dangerous" pattern that
was fixed in the prior commit (wherein a negative return
value is promoted to unsigned), though it is not dangerous
here because our "41" is a constant, not an unsigned
variable.

But we should convert it anyway to avoid modeling a
dangerous construct.

Signed-off-by: Jeff King <redacted>
---
 builtin/get-tar-commit-id.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

I kind of disagree with calling this dangerous (and I think that is
what you alluded to above by putting it in quotes), but I like the
postimage more than the preimage.

Right, this instance is fine, but the pattern of using "<" is not. If
you swapped out "41" for:

  size_t len = 41;

then it would be a bug. Which I think would surprise most people.

The variable 'n' could be eliminated to simplify this further.  I
realize that would go against the spirit of this patch, but (1) it's
on-topic for the patch, since it is another ssize_t vs constant
comparison and (2) as mentioned elsewhere in this thread, it's a very
common idiom with read_in_full.  If we want to eliminate it then we
could introduce a separate helper to distinguish between
read_this_much_i_mean_it and read_this_much_or_to_eof.

Yes, I noticed that, too, after you brought up read_in_full() as a
potential source of problems. But I would rather deal with
read_in_full() separately on top. Can you do it as a separate patch on
top (possibly with other read_in_full() cleanups, though I think this is
the only "<" case that exists).

-Peff

On Wed, Sep 13, 2017 at 02:14:30PM -0700, Jonathan Nieder wrote:

quoted

       I really wish every "write_in_full()" user would just
       check against "<0" now, but this fixes the nasty and
       stupid ones.

Ok, you convinced me.

Should we add a comment to cache.h as well encouraging this?

I'd be OK with a comment, though I don't know that it's strictly
necessary. It looks like most of it was just cargo-culted, so removing
the offending examples is sufficient.

quoted

[1] A careful reader may notice there is one way that
    write_in_full() can return a different value. If we ask
    write() to write N bytes and get a return value that is
    _larger_ than N, we could return a larger total. But
    besides the fact that this would imply a totally broken
    version of write(), it would already invoke undefined
    behavior. Our internal remaining counter is an unsigned
    size_t, which means that subtracting too many byte will
    wrap it around to a very large number. So we'll instantly
    begin reading off the end of the buffer, trying to write
    gigabytes (or petabytes) of data.

This footnote just leaves me more confused, since as you mention,
write() never would return a value greater than N.  Are you saying we
need to defend against a broken platform where that isn't true?

No, I'm saying that my claim that write_in_full() can only return two
values (-1 and the original length) is not strictly true. But that it
doesn't matter in practice.

I don't think we need to defend against such a broken platform, but I
didn't want anybody reading the claim to say "aha, you forgot this
case". It is a case that does not matter.

-Peff

On Wed, Sep 13, 2017 at 02:20:35PM -0700, Jonathan Nieder wrote:

quoted

--- a/notes-merge.c
+++ b/notes-merge.c

@@ -302,7 +302,7 @@ static void write_buf_to_worktree(const struct object_id *obj,
 	fd = xopen(path, O_WRONLY | O_EXCL | O_CREAT, 0666);
 
 	while (size > 0) {
-		long ret = write_in_full(fd, buf, size);
+		ssize_t ret = write_in_full(fd, buf, size);
 		if (ret < 0) {
 			/* Ignore epipe */
 			if (errno == EPIPE)

				break;
			die_errno("notes-merge");
		} else if (!ret) {
			die("notes-merge: disk full?");
		}

These three lines are dead code.  How about the following, e.g. for
squashing in?

Thanks, I didn't notice that.

I'd actually prefer it as a separate patch, since it needs explained
separately.

-Peff

On Wed, Sep 13, 2017 at 02:25:28PM -0700, Jonathan Nieder wrote:

quoted

Let's flip them to follow the usual write() conventions and
update all callers. As these are local to config.c, it's
unlikely that we'd have new callers in any topics in flight
(which would be silently broken by our change). But just to
be on the safe side, let's rename them to just
write_section() and write_pairs().  That also accentuates
their relationship with write().

Signed-off-by: Jeff King <redacted>

The caller only cares if it succeeded, right?  Could this return
the customary 0 vs -1 instead of the number of bytes written?

Yes, it could. I went with "follow the conventions of write()" because
these are used in a big chain of write() calls (well, really
write_in_full). But given the current callers, it does not matter either
way.

Thanks for reviewing the series, and sorry if my comments have been a
bit terse. I'm trying to clear my pile before going out of town for a
few days (which I admit may have contributed to my desire for you to
prepare patches on top).

But either way, don't expect a re-roll until next week at the earliest.

-Peff

On 15/09/17 01:37, Jeff King wrote:

On Thu, Sep 14, 2017 at 12:31:38AM +0100, Ramsay Jones wrote:

quoted

I just tried it again tonight; the current master branch has 3532
warnings when compiled with -Wextra, 1409 of which are -Wsign-compare
warnings. After applying the patch below, those numbers are reduced
by 344 to 3188/1065.

I'd love it if we were clean on -Wextra. My big question is many
contortions we'd have to go through in the code. I don't mind at all if
we're actually cleaning as we go (e.g., using types of the correct
signedness, or preventing possible funny interactions). I'm just worried
it will turn into a bunch of noisy casts.

Hmm, my memory is not what it was, ... ;-) However, I seem to recall
that most of the changes were "improvements", with only a (relatively)
few scattering of casts (behind existing macros - namely the OPTION
macros). (Oh, wait, I think 'unused' parameters etc., was another
problem area).

The patch you showed seems like an improvement to the code, but I don't
know if that would be the case for all of them. :)

Yes, I've had that patch (and others like it) hanging around for
years! (perhaps it's time to dig through all those old branches)

ATB,
Ramsay Jones