Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

journal: deal better with reading from zeroed out journal mmaps #15557

Merged
merged 9 commits into from May 21, 2020

Conversation

poettering
Copy link
Member

A fix for #14943

When accessing journal files we generally are fine when values change
beneath our feet, while we are looking at them, as long as they change
from something valid to zero. This is required since we nowadays
forcibly unallocate journal files on vacuuming, to ensure they are
actually released.

However, we need to make sure that the validity checks we enforce are
done on suitable copies of the fields in the file. Thus provide a macro
that forces a copy, and disallows the compiler from merging our copy
with the actually memory where it is from.
The value might change beneath what we do, and hence let's avoid any
chance of underflow.
Mappings canbe replaced by all zeroes under our feet if vacuuming
decides to unallocate some file. Hence let's not check for this kind of
stuff in an assert.

(Typically, we should genreate runtime errors in this case, in
particular EBADMSG, which the callers generally look for. But in this
case this is just an extra precaution check anyway, so let's just remove
it.)
… arithmetics on them

Our journal code is generally supposed to be written in a fashion that
the underlying file can be deallocated any time, i.e. our mmap of it
suddenly becomes all zeroes. The idea is that we catch that when parsing
everything. For that to work safely we need to make sure that when doing
arithmetics or comparisons on values read from the map we don't run into
TTOCTTOU issues when determining validity. Hence we need to copy out the
values before use and operate on the copies. This requires some special
care since the C compiler could suppress our copies as optimization.
Hence use the new READ_NOW() macro to force a copy by using memcpy(),
and use it whenever we start doing an arithmetic operation on it, or
validity checking of multiple steps.

Fixes: systemd#14943
@anitazha
Copy link
Member

LGTM

@anitazha anitazha merged commit b10ceb4 into systemd:master May 21, 2020
@oracle1992
Copy link

Hi @poettering we are getting below crash in our CI environment with systemd v244, so could you please confirm whether this PR will fix it? If yes then I will send back port of these PR to v244 as well:
(gdb) bt full
#0 __GI_abort () at abort.c:107
act = {__sigaction_handler = {sa_handler = 0x0, sa_sigaction = 0x0}, sa_mask = {__val = {
18446744073709551615 <repeats 16 times>}}, sa_flags = 0, sa_restorer = 0x0}
sigs = {__val = {32, 0 <repeats 15 times>}}
#1 0x0000007f87d2900c in log_assert_failed_realm (realm=realm@entry=LOG_REALM_SYSTEMD,
text=text@entry=0x7f87d41f30 "p > 0",
file=file@entry=0x7f87d41e1f "src/journal/journal-file.c", line=line@entry=2442,
func=func@entry=0x7f87d42fe0 <PRETTY_FUNCTION.13000> "test_object_seqnum")
at ../git/src/basic/log.c:809
No locals.
#2 0x0000007f87d0e5e8 in test_object_seqnum (f=0x217c, p=0, needle=8572)
at ../git/src/journal/journal-file.c:2442
o =
r =
PRETTY_FUNCTION = "test_object_seqnum"
#3 test_object_seqnum (f=f@entry=0x7f78012030, p=p@entry=0, needle=needle@entry=8572)
at ../git/src/journal/journal-file.c:2437
o = 0x7f8524a0a8
r =
PRETTY_FUNCTION = "test_object_seqnum"
#4 0x0000007f87d0fa84 in generic_array_bisect_plus_one (idx=0x0, offset=0x7f8524a210, ret=0x0,
direction=DIRECTION_DOWN, test_object=0x7f87d0e528 <test_object_seqnum>, needle=8572, n=1,
first=0, extra=0, f=0x7f78012030) at ../git/src/journal/journal-file.c:2376
r =
step_back =
o = 0x173318
r =
--Type for more, q to quit, c to continue without paging--
step_back =
o =
PRETTY_FUNCTION = "generic_array_bisect_plus_one"
#5 generic_array_bisect_plus_one (f=0x7f78012030, extra=0, first=0, n=1, needle=8572,
test_object=0x7f87d0e528 <test_object_seqnum>, direction=DIRECTION_DOWN, ret=0x0,
offset=0x7f8524a210, idx=0x0) at ../git/src/journal/journal-file.c:2352
r =
o =
PRETTY_FUNCTION = "generic_array_bisect_plus_one"
#6 0x0000007f87d10f84 in journal_file_move_to_entry_by_seqnum_for_data (f=f@entry=0x7f78012030,
data_offset=, seqnum=8572, direction=direction@entry=DIRECTION_DOWN,
ret=ret@entry=0x0, offset=offset@entry=0x7f8524a210) at ../git/src/basic/sparse-endian.h:83
d = 0x7f77a0cce8
r =
PRETTY_FUNCTION = "journal_file_move_to_entry_by_seqnum_for_data"
#7 0x0000007f87d15990 in find_location_for_match (j=j@entry=0x7f78000bc0,
m=m@entry=0x7f78001fa0, f=f@entry=0x7f78012030, direction=direction@entry=DIRECTION_DOWN,
ret=ret@entry=0x0, offset=offset@entry=0x7f8524a210) at ../git/src/journal/sd-journal.c:605
dp = 52456
r =
PRETTY_FUNCTION = "find_location_for_match"
#8 0x0000007f87d15838 in find_location_for_match (j=j@entry=0x7f78000bc0,
m=m@entry=0x7f78001f50, f=f@entry=0x7f78012030, direction=direction@entry=DIRECTION_DOWN,
ret=ret@entry=0x0, offset=offset@entry=0x7f8524a2a0) at ../git/src/journal/sd-journal.c:626
cp = 7161343769125990453
np = 0
n = 0x6362343731363035
--Type for more, q to quit, c to continue without paging--
i = 0x7f78001fa0
r =
PRETTY_FUNCTION = "find_location_for_match"
#9 0x0000007f87d156f8 in find_location_for_match (j=j@entry=0x7f78000bc0,
m=m@entry=0x7f78001f00, f=f@entry=0x7f78012030, direction=direction@entry=DIRECTION_DOWN,
ret=ret@entry=0x0, offset=offset@entry=0x7f8524a330) at ../git/src/journal/sd-journal.c:664
cp = 547474115520
i = 0x7f78001f50
np = 0
r =
PRETTY_FUNCTION = "find_location_for_match"
#10 0x0000007f87d15838 in find_location_for_match (j=j@entry=0x7f78000bc0,
m=m@entry=0x7f78001eb0, f=f@entry=0x7f78012030, direction=direction@entry=DIRECTION_DOWN,
ret=ret@entry=0x0, offset=offset@entry=0x7f8524a3c0) at ../git/src/journal/sd-journal.c:626
cp = 1000000
np = 0
n = 0xf4240
i = 0x7f78001f00
r =
PRETTY_FUNCTION = "find_location_for_match"
#11 0x0000007f87d156f8 in find_location_for_match (j=j@entry=0x7f78000bc0, m=0x7f78001e60,
f=f@entry=0x7f78012030, direction=direction@entry=DIRECTION_DOWN, ret=ret@entry=0x7f8524a478,
offset=offset@entry=0x7f8524a480) at ../git/src/journal/sd-journal.c:664
cp = 547694617584
i = 0x7f78001eb0
np = 0
r =
--Type for more, q to quit, c to continue without paging--
PRETTY_FUNCTION = "find_location_for_match"
#12 0x0000007f87d1725c in find_location_with_matches (offset=0x7f8524a480, ret=0x7f8524a478,
direction=DIRECTION_DOWN, f=0x7f78012030, j=0x7f78000bc0)
at ../git/src/journal/sd-journal.c:709
r =
r =
PRETTY_FUNCTION =
#13 next_beyond_location (direction=DIRECTION_DOWN, f=0x7f78012030, j=0x7f78000bc0)
at ../git/src/journal/sd-journal.c:769
c = 0x555ae98fa0 <journal_thread_impl+1240>
cp = 547694617776
n_entries =
r =
c =
cp =
n_entries =
r =
PRETTY_FUNCTION = "next_beyond_location"
found =
k =
#14 real_journal_next (j=0x7f78000bc0, direction=direction@entry=DIRECTION_DOWN)
at ../git/src/journal/sd-journal.c:823
f = 0x7f78012030
found =
new_file = 0x0
i = 8
n_files = 9
--Type for more, q to quit, c to continue without paging--
files = 0x7f78002030
o = 0x7f8524a4b0
r =
PRETTY_FUNCTION = "real_journal_next"
func = "real_journal_next"
#15 0x0000007f87d18e10 in sd_journal_next (j=)
at ../git/src/journal/sd-journal.c:860
No locals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

when reading journal file, it lacks condition judgment of "object.size=0"
3 participants