This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6554

cleanup: always `seek(0, io.SEEK_END)` after open in append mode before tell()
AbandonedPublic

Authored by durin42 on Jun 20 2019, 2:29 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

Consider the program:

#include <stdio.h>

int main() {
  FILE *f = fopen("narf", "w");
  fprintf(f, "narf\n");
  fclose(f);

  f = fopen("narf", "a");
  printf("%ld\n", ftell(f));
  fprintf(f, "troz\n");
  printf("%ld\n", ftell(f));

  return 0;
}

on macOS, FreeBSD, and Linux with glibc, this program prints

5
10

but on musl libc (Alpine Linux and probably others) this prints

0
10

By my reading of
https://pubs.opengroup.org/onlinepubs/009695399/functions/fopen.html
this is technically correct, specifically:

Opening a file with append mode (a as the first character in the
mode argument) shall cause all subsequent writes to the file to be
forced to the then current end-of-file, regardless of intervening
calls to fseek().

in other words, the file position doesn't really matter in append-mode
files, and we can't depend on it being at all meaningful unless we
perform a seek() before tell() after open(..., 'a'). Experimentally
after a .write() we can do a .tell() and it'll always be reasonable,
but I'm unclear from reading the specification if that's a smart thing
to rely on.

These callsites were identified by applying the next changeset and
then fixing new crashes.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

durin42 created this revision.Jun 20 2019, 2:29 PM
durin42 added a comment.Jun 20 2019, 2:42 PM

Note that I mailed a version to the list for stable. This doesn't quite cleanly apply there, so I made the change twice so I could write the next change without waiting for a merge.

durin42 abandoned this revision.Jul 8 2019, 1:15 PM

Revision Contents
Changeset List

PathPackages
Mmercurial/branchmap.py (3 lines)
Mmercurial/obsolete.py (2 lines)
Mmercurial/tags.py (4 lines)

Diff 15617

mercurial/branchmap.py

# branchmap.py - logic to computes, maintain and stores branchmap for local repo # branchmap.py - logic to computes, maintain and stores branchmap for local repo
# #
# Copyright 2005-2007 Matt Mackall <mpm@selenic.com> # Copyright 2005-2007 Matt Mackall <mpm@selenic.com>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import io
import struct import struct
from .node import ( from .node import (
bin, bin,
hex, hex,
nullid, nullid,
nullrev, nullrev,
) )
finally: finally:
if wlock is not None: if wlock is not None:
wlock.release() wlock.release()
def _writenames(self, repo): def _writenames(self, repo):
""" write the new branch names to revbranchcache """ """ write the new branch names to revbranchcache """
if self._rbcnamescount != 0: if self._rbcnamescount != 0:
f = repo.cachevfs.open(_rbcnames, 'ab') f = repo.cachevfs.open(_rbcnames, 'ab')
f.seek(0, io.SEEK_END)
if f.tell() == self._rbcsnameslen: if f.tell() == self._rbcsnameslen:
f.write('\0') f.write('\0')
else: else:
f.close() f.close()
repo.ui.debug("%s changed - rewriting it\n" % _rbcnames) repo.ui.debug("%s changed - rewriting it\n" % _rbcnames)
self._rbcnamescount = 0 self._rbcnamescount = 0
self._rbcrevslen = 0 self._rbcrevslen = 0
if self._rbcnamescount == 0: if self._rbcnamescount == 0:
# before rewriting names, make sure references are removed # before rewriting names, make sure references are removed
repo.cachevfs.unlinkpath(_rbcrevs, ignoremissing=True) repo.cachevfs.unlinkpath(_rbcrevs, ignoremissing=True)
f = repo.cachevfs.open(_rbcnames, 'wb') f = repo.cachevfs.open(_rbcnames, 'wb')
f.write('\0'.join(encoding.fromlocal(b) f.write('\0'.join(encoding.fromlocal(b)
for b in self._names[self._rbcnamescount:])) for b in self._names[self._rbcnamescount:]))
self._rbcsnameslen = f.tell() self._rbcsnameslen = f.tell()
f.close() f.close()
self._rbcnamescount = len(self._names) self._rbcnamescount = len(self._names)
def _writerevs(self, repo, start): def _writerevs(self, repo, start):
""" write the new revs to revbranchcache """ """ write the new revs to revbranchcache """
revs = min(len(repo.changelog), len(self._rbcrevs) // _rbcrecsize) revs = min(len(repo.changelog), len(self._rbcrevs) // _rbcrecsize)
with repo.cachevfs.open(_rbcrevs, 'ab') as f: with repo.cachevfs.open(_rbcrevs, 'ab') as f:
f.seek(0, io.SEEK_END)
if f.tell() != start: if f.tell() != start:
repo.ui.debug("truncating cache/%s to %d\n" % (_rbcrevs, start)) repo.ui.debug("truncating cache/%s to %d\n" % (_rbcrevs, start))
f.seek(start) f.seek(start)
if f.tell() != start: if f.tell() != start:
start = 0 start = 0
f.seek(start) f.seek(start)
f.truncate() f.truncate()
end = revs * _rbcrecsize end = revs * _rbcrecsize
f.write(self._rbcrevs[start:end]) f.write(self._rbcrevs[start:end])
self._rbcrevslen = revs self._rbcrevslen = revs

mercurial/obsolete.py

The header is followed by the markers. Marker format depend of the version. See The header is followed by the markers. Marker format depend of the version. See
comment associated with each format for details. comment associated with each format for details.
""" """
from __future__ import absolute_import from __future__ import absolute_import
import errno import errno
import hashlib import hashlib
import io
import struct import struct
from .i18n import _ from .i18n import _
from . import ( from . import (
encoding, encoding,
error, error,
node, node,
obsutil, obsutil,
getsuccessors = self.successors.get getsuccessors = self.successors.get
new = [] new = []
for m in markers: for m in markers:
if m not in getsuccessors(m[0], ()) and m not in known: if m not in getsuccessors(m[0], ()) and m not in known:
known.add(m) known.add(m)
new.append(m) new.append(m)
if new: if new:
f = self.svfs('obsstore', 'ab') f = self.svfs('obsstore', 'ab')
f.seek(0, io.SEEK_END)
try: try:
offset = f.tell() offset = f.tell()
transaction.add('obsstore', offset) transaction.add('obsstore', offset)
# offset == 0: new file - add the version header # offset == 0: new file - add the version header
data = b''.join(encodemarkers(new, offset == 0, self._version)) data = b''.join(encodemarkers(new, offset == 0, self._version))
f.write(data) f.write(data)
finally: finally:
# XXX: f.close() == filecache invalidation == obsstore rebuilt. # XXX: f.close() == filecache invalidation == obsstore rebuilt.

mercurial/tags.py

# tags.py - read tag info from local repository # tags.py - read tag info from local repository
# #
# Copyright 2009 Matt Mackall <mpm@selenic.com> # Copyright 2009 Matt Mackall <mpm@selenic.com>
# Copyright 2009 Greg Ward <greg@gerg.ca> # Copyright 2009 Greg Ward <greg@gerg.ca>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
# Currently this module only deals with reading and caching tags. # Currently this module only deals with reading and caching tags.
# Eventually, it could take care of updating (adding/removing/moving) # Eventually, it could take care of updating (adding/removing/moving)
# tags too. # tags too.
from __future__ import absolute_import from __future__ import absolute_import
import errno import errno
import io
from .node import ( from .node import (
bin, bin,
hex, hex,
nullid, nullid,
nullrev, nullrev,
short, short,
) )
fp.close() fp.close()
prevtags = '' prevtags = ''
if local: if local:
try: try:
fp = repo.vfs('localtags', 'r+') fp = repo.vfs('localtags', 'r+')
except IOError: except IOError:
fp = repo.vfs('localtags', 'a') fp = repo.vfs('localtags', 'a')
fp.seek(0, io.SEEK_END)
else: else:
prevtags = fp.read() prevtags = fp.read()
# local tags are stored in the current charset # local tags are stored in the current charset
writetags(fp, names, None, prevtags) writetags(fp, names, None, prevtags)
for name in names: for name in names:
repo.hook('tag', node=hex(node), tag=name, local=local) repo.hook('tag', node=hex(node), tag=name, local=local)
return return
try: try:
fp = repo.wvfs('.hgtags', 'rb+') fp = repo.wvfs('.hgtags', 'rb+')
except IOError as e: except IOError as e:
if e.errno != errno.ENOENT: if e.errno != errno.ENOENT:
raise raise
fp = repo.wvfs('.hgtags', 'ab') fp = repo.wvfs('.hgtags', 'ab')
fp.seek(0, io.SEEK_END)
else: else:
prevtags = fp.read() prevtags = fp.read()
# committed tags are stored in UTF-8 # committed tags are stored in UTF-8
writetags(fp, names, encoding.fromlocal, prevtags) writetags(fp, names, encoding.fromlocal, prevtags)
fp.close() fp.close()
lock = repo.wlock(wait=False) lock = repo.wlock(wait=False)
except error.LockError: except error.LockError:
repo.ui.log('tagscache', 'not writing .hg/cache/%s because ' repo.ui.log('tagscache', 'not writing .hg/cache/%s because '
'lock cannot be acquired\n' % (_fnodescachefile)) 'lock cannot be acquired\n' % (_fnodescachefile))
return return
try: try:
f = repo.cachevfs.open(_fnodescachefile, 'ab') f = repo.cachevfs.open(_fnodescachefile, 'ab')
f.seek(0, io.SEEK_END)
try: try:
# if the file has been truncated # if the file has been truncated
actualoffset = f.tell() actualoffset = f.tell()
if actualoffset < self._dirtyoffset: if actualoffset < self._dirtyoffset:
self._dirtyoffset = actualoffset self._dirtyoffset = actualoffset
data = self._raw[self._dirtyoffset:] data = self._raw[self._dirtyoffset:]
f.seek(self._dirtyoffset) f.seek(self._dirtyoffset)
f.truncate() f.truncate()