json config files get corrupted when using Python 2.7.6 on Linux

[Hans de Goede]

Hi,

I noticed when running calibre 1.28.0 on Fedora-21 that I got the following messages when starting calibre from a terminal:

Traceback (most recent call last):
  File "/usr/lib64/calibre/calibre/utils/config.py", line 298, in refresh
    d = self.raw_to_object(raw) if raw.strip() else {}
  File "/usr/lib64/calibre/calibre/utils/config.py", line 389, in raw_to_object
    return json.loads(raw.decode('utf-8'), object_hook=from_json)
  File "/usr/lib64/python2.7/json/__init__.py", line 351, in loads
    return cls(encoding=encoding, **kw).decode(s)
  File "/usr/lib64/python2.7/json/decoder.py", line 368, in decode
    raise ValueError(errmsg("Extra data", s, end, len(s)))
ValueError: Extra data: line 19 column 2 - line 37 column 2 (char 349 - 698)
loaded the Generic plugin

And that my preferences were no longer honored / remembered.

I've done some debugging and the problem is that ie gui.json gets any new config settings appended rather then that then contents gets overwritten. IE gui.json looks like this after one run (abbreviated):

{
  "quick_start_guide_added": true,
  "library_usage_stats": {
    "/home/hans/Calibre Library": 1
  },
}{
  "quick_start_guide_added": true,
  "library_usage_stats": {
    "/home/hans/Calibre Library": 1
  },
}

I've tracked this down to calibre/utils/lock.py ExclusiveFile,__enter__
doing (abbreviated again):

self.file = open(self.path, 'a+b')

When running under anything but windows. Note the a+ stands for append mode, now
calibre/utils/config.py: XMLConfig.commit does:
f.seek(0)
f.truncate()

On the file, but this seems to no longer lead to the desired result, given what the python docs say:
http://docs.python.org/2/library/stdtypes.html#bltin-file-objects

"file.seek(offset[, whence]) ... Note that if the file is opened for appending (mode 'a' or 'a+'), any seek() operations will be undone at the next write."

This is not really surprising as a truncate is a write. As the calibre code used to work fine until
recently, I can only assume that truncate used to be a loophole around this limitation, and
that this has now been fixed.

So I've audited all the users of ExclusiveFile in calibre, and whenever the write they always
first do a truncate. So there really is no need to use "a" as mode when opening the file,
so replacing the open in ExclusiveFile,__enter__ with:

self.file = open(self.path, 'r+b')

Fixes this.

Thanks & Regards,

Hans

[Kovid Goyal (kovid)]

I cannot replicate this with python 2.7.6 on my gentoo system. See the test below:

$ python -c "f = open('/tmp/test', 'wb'); f.write('abcd'); f.close(); f = open('/tmp/test', 'a+b'); f.seek(0); f.truncate(); f.write('abcd'); f.close(); print (open('/tmp/test').read()); import sys; print sys.version"
abcd
2.7.6 (default, Dec 3 2013, 21:15:46)
[GCC 4.8.2]

As you can see, seek(0) followed by truncate() works as expected on python 2.7.6.

IIRC, the python file object is just a thin wrapper around the libc FILE* pointer. So I suspect that this has something to do with a change in how truncate works in whatever C library your system is using. It seems rather odd that something as fundamental as the C library would change the behavior of something as widely used as truncate(), but...

You cannot change a+b to r+b. That would break when the file being used does not exist. That was the entire reason for using a+b in the first place. Using r+b will break, for example, the set() method of the Config class in config_base.py

The only way to accommodate your broken libc would be to first try to open the file in r+b and then fallback to w+b. There is a race condition in that. It annoys me to be forced to use code that is vulnerable to a race condition just to accommodate one distro's broken libc.

It may be possible to open in r+b and promote to w+b using fcntl, I will have to look into that.

What version/implementation of libc is your system using?

[Kovid Goyal (kovid)]

Sorry, make that open in a+b and promote to r+b using fcntl.

[Kovid Goyal (kovid)]

And reading the man page for O_APPEND it states:

" O_APPEND
              The file is opened in append mode. Before each write(2), the file offset is positioned at the end of the file, as if with lseek(2)."

It specifically says that write() is affected by O_APPEND not truncate()

[Kovid Goyal (kovid)]

Fixed in branch master. The fix will be in the next release. calibre is usually released every Friday.

 status fixreleased

[Hans de Goede (j-w-r-degoede)]

Hi,

Thanks for the quick fix. I've just tested it and I can confirm it works.

As for your various comments on this:

1) You're right r+ won't work for non existing files, my bad.
2) Fedora-21 is using glibc-2.19.90
3) As for append mode affecting truncate, it could also have something to-do with the C-level append taking a size argument, and for the python wrapper the size argument is optional, and if size is not passed in then "The size defaults to the current position." So the problem may also have something to do with how the python truncate wrapper determines the "current position" in combination with the append flag.

Anyways the fix you've done looks good and sensible.

Regards,

Hans