[2.7] bpo-36149 Fix potential use of uninitialized memory in cPickle

[Yhg1s]

Fix potential use of uninitialized memory in cPickle when reading truncated pickles from FILE*s.

https://bugs.python.org/issue36149

[benjaminp]

This is exactly why you shouldn't put all your logic in the "condition" of one if-statement.

[benjaminp]

Isn't this redundant with the check on line 593?

[Yhg1s]

I don't know, is it? I can never remember the guaranteed semantics of FILE*s, feof() and functions that return EOF (which can signal error). Since the original function used this, I kept this it in.

[benjaminp]

My manpage says getc() returns EOF on end-of-file and error, so that's why I believe this is subsumed by the next call.

[benjaminp]

I find it a little weird that this section is down here when the only way you can get to it is to jump out of a twice nested loop above.

[Yhg1s]

In my mind the end of the function is the natural place to put this, because it makes clear that the outer loop doesn't end without this bit getting executed (barring error-returns).

[benjaminp]

self->buf[i++] = (newchar == EOF) ? '\0' : newchar maybe?

[Yhg1s]

Didn't you just complain about too much logic in a single statement? :)

[Yhg1s]

(I'm happy to change it if you prefer, but since this code is all blisfully gone in 3 and it isn't exactly performance-critical I'm not sure it really matters...)

[benjaminp]

I think you could resolve all my nits by writing:

while (i < (self->buf_size - 1)) {
    int newchar = getc(self->fp);
    int done = newchar == EOF || newchar == '\n';
    if (done) {
        self->buf[i] = '\0';
        *s = self->buf;
        return i;
    }
    self->buf[i] = newchar;
    i++;
}

Do you think that's clearer?

[benjaminp]

(You could even bring back the for loop.)

[Yhg1s]

The loop you're proposing makes it not write the newline to self->buf. I'm not sure if that really matters (because it depends on what the callers of readline_file do) but it certainly changes what readline_file does.

[benjaminp]

Ooops. Maybe

while (i < (self->buf_size - 1)) {
    int newchar = getc(self->fp);
    if (newchar != EOF) {
        self->buf[i++] = newchar;
    }
    if (newchar == EOF || newchar == '\n') {
        self->buf[i] = '\0';
        *s = self->buf;
        return i;
    }
}

then?

[Yhg1s]

Fine, if you want to hide the function exit in the inner loop, there you go :)

[benjaminp]

We must just have different C aesthetics.

[bedevere-bot]

@Yhg1s: Please replace # with GH- in the commit message next time. Thanks!

[bedevere-bot]

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Windows7 SP1 2.7 has failed when building commit d9bf7f4.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/68/builds/238) and take a look at the build logs.
4. Check if the failure is related to this commit (d9bf7f4) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/68/builds/238

Click to see traceback logs

From https://github.com/python/cpython
 * branch            2.7        -> FETCH_HEAD
Reset branch '2.7'

Could Not Find C:\buildbot.python.org\2.7.kloth-win64\build\Lib\*.pyc
The system cannot find the file specified.

test test_urllibnet failed -- multiple errors occurred; run in verbose mode for details
Resource 'http://www.example.com' is not available
C:\buildbot.python.org\2.7.kloth-win64\build\lib\distutils\msvc9compiler.py:120: DeprecationWarning: decoding Unicode is not supported in 3.x
  s = dec("mbcs")
Resource 'ipv6.google.com' is not available
C:\buildbot.python.org\2.7.kloth-win64\build\lib\threading.py:846: DeprecationWarning: sys.exc_clear() not supported in 3.x; use except clauses
  self.__exc_clear()
'python2.4' is not recognized as an internal or external command,
operable program or batch file.
'python2.5' is not recognized as an internal or external command,
operable program or batch file.
'python2.6' is not recognized as an internal or external command,
operable program or batch file.
'python2.7' is not recognized as an internal or external command,
operable program or batch file.
    a DOS box should flash briefly ...
C:\buildbot.python.org\2.7.kloth-win64\build\lib\shutil.py:78: UnicodeWarning: Unicode equal comparison failed to convert both arguments to Unicode - interpreting them as being unequal
  os.path.normcase(os.path.abspath(dst)))
C:\buildbot.python.org\2.7.kloth-win64\build\lib\multiprocessing\pool.py:117: DeprecationWarning: can't pickle function objects
  put((job, i, result))
C:\buildbot.python.org\2.7.kloth-win64\build\lib\multiprocessing\pool.py:117: DeprecationWarning: can't pickle function objects
  put((job, i, result))
Berkeley DB 4.7.25: (May 15, 2008)
Test path prefix:  c:\users\buildbot\appdata\local\temp\z-test_bsddb3-4576
XXX: timeout happened beforestartup was confirmed - see issue 3892
test test_urllibnet failed -- multiple errors occurred

[Yhg1s]

FYI, the amd64 windows7 buidbot failure appears unrelated to this change (it's a test_urllibnet timeout failure).