This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1948

lazymanifest: avoid reading uninitialized memory
ClosedPublic

Authored by quark on Jan 30 2018, 9:42 PM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHG113a30b87716: lazymanifest: avoid reading uninitialized memory
Summary

I got errors running tests with clang UBSAN [1] enabled. One of them is:

--- test-dirstate.t
+++ test-dirstate.t.err
@@ -85,9 +85,115 @@
   $ echo "[extensions]" >> .hg/hgrc
   $ echo "dirstateex=../dirstateexception.py" >> .hg/hgrc
   $ hg up 0
-  abort: simulated error while recording dirstateupdates
-  [255]
+  mercurial/cext/manifest.c:781:13: runtime error: load of value 190, which is not a valid value for type 'bool'
+      #0 0x7f668a8cf748 in lazymanifest_diff mercurial/cext/manifest.c:781
+      #1 0x7f6692fc1dc4 in call_function Python-2.7.11/Python/ceval.c:4350
+      .......
+  SUMMARY: UndefinedBehaviorSanitizer: invalid-bool-load mercurial/cext/manifest.c:781:13 in
+  [1]
   $ hg log -r . -T '{rev}\n'
   1
   $ hg status
-  ? a

While the code is not technically wrong, but switching the condition would
make clang UBSAN happy. So let's do it.

The uninitialized memory could come from, for example, lazymanifest_copy
allocates self->maxlines items but only writes the first self->lines
items.

[1]: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html

Test Plan

Run test-dirstate.t with UBSAN and it no longer reports the issue.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

quark created this revision.Jan 30 2018, 9:42 PM
quark edited the summary of this revision. (Show Details)Jan 30 2018, 9:47 PM
quark edited the test plan for this revision. (Show Details)
quark edited the summary of this revision. (Show Details)
indygreg accepted this revision.Jan 30 2018, 11:27 PM
This revision is now accepted and ready to land.Jan 30 2018, 11:27 PM
indygreg added a comment.Jan 30 2018, 11:34 PM

FWIW, I could not hg phabread | hg import - this revision because hg import's patch parser got confused by the inline patch-looking content of the commit message. Good times.

Closed by commit rHG113a30b87716: lazymanifest: avoid reading uninitialized memory (authored by quark). · Explain WhyJan 30 2018, 11:35 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/cext/manifest.c (4 lines)

Diff 5020

mercurial/cext/manifest.c

while (sneedle != self->numlines || oneedle != other->numlines) { while (sneedle != self->numlines || oneedle != other->numlines) {
line *left = self->lines + sneedle; line *left = self->lines + sneedle;
line *right = other->lines + oneedle; line *right = other->lines + oneedle;
int result; int result;
PyObject *key; PyObject *key;
PyObject *outer; PyObject *outer;
/* If we're looking at a deleted entry and it's not /* If we're looking at a deleted entry and it's not
* the end of the manifest, just skip it. */ * the end of the manifest, just skip it. */
if (left->deleted && sneedle < self->numlines) { if (sneedle < self->numlines && left->deleted) {
sneedle++; sneedle++;
continue; continue;
} }
if (right->deleted && oneedle < other->numlines) { if (oneedle < other->numlines && right->deleted) {
oneedle++; oneedle++;
continue; continue;
} }
/* if we're at the end of either manifest, then we /* if we're at the end of either manifest, then we
* know the remaining items are adds so we can skip * know the remaining items are adds so we can skip
* the strcmp. */ * the strcmp. */
if (sneedle == self->numlines) { if (sneedle == self->numlines) {
result = 1; result = 1;