[Bug 5541] New: mercurial journal extension uses incorrect sequence of fread/fwrite

[mercurial-bugs at mercurial-scm.org]

https://bz.mercurial-scm.org/show_bug.cgi?id=5541

            Bug ID: 5541
           Summary: mercurial journal extension uses incorrect sequence of
                    fread/fwrite
           Product: Mercurial
           Version: 4.1.2
          Hardware: Other
                OS: Other
            Status: UNCONFIRMED
          Severity: bug
          Priority: wish
         Component: Mercurial
          Assignee: bugzilla at mercurial-scm.org
          Reporter: apyhalov at gmail.com
                CC: mercurial-devel at mercurial-scm.org

In mercurial journal extension we see the following code:

 with vfs('namejournal', mode='a+b', atomictemp=True) as f:
                f.seek(0, os.SEEK_SET)
                # Read just enough bytes to get a version number (up to 2
                # digits plus separator)
                version = f.read(3).partition('\0')[0]
                if version and version != str(storageversion):
                    # different version of the storage. Exit early (and not
                    # write anything) if this is not a version we can handle or
                    # the file is corrupt. In future, perhaps rotate the file
                    # instead?
                    self.ui.warn(
                        _("unsupported journal file version '%s'\n") % version)
                    return
                if not version:
                    # empty file, write version first
                    f.write(str(storageversion) + '\0')
                f.seek(0, os.SEEK_END)
                f.write(str(entry) + '\0')

It is not correct, at least for illumos, as (from fopen man page):

       When a file is opened with update mode (+ as the second or third
       character in the mode argument), both input and output may be performed
       on the associated stream. However, output must not be directly followed
       by input without an intervening call to fflush(3C) or to a file
       positioning function ( fseek(3C), fsetpos(3C) or rewind(3C)), and input
       must not be directly followed by output without an intervening call to
       a file positioning function, unless the input operation encounters end-
       of-file.

The same is stated in gnu fopen man page:

Reads and writes may be intermixed on read/write streams in any order.  Note
that ANSI C requires that a file positioning function intervene between output
and input, unless an input operation encounters  end-of-file.   (If
this  condition  is  not  met,  then a read is allowed to return the result of
writes other than the most recent.)  Therefore it is good practice (and indeed
sometimes necessary under Linux) to put an fseek(3) or fgetpos(3)
operation between write and read operations on such a stream.  This operation
may be an apparent no-op (as in fseek(..., 0L, SEEK_CUR) called for its
synchronizing side effect).

And here we see fread() folowed by fwrite(). 
This leads to missing first fwrite() result from namedjournal (leading 0 is not
recorded). 
To correct code we could use something like:

            if not version:
                    f.seek(0, os.SEEK_SET)
                    # empty file, write version first
                    f.write(str(storageversion) + '\0')

-- 
You are receiving this mail because:
You are on the CC list for the bug.