Page MenuHomeFreeBSD

diff: Fix integer overflow.
ClosedPublic

Authored by des on Jul 27 2024, 5:42 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 19, 11:02 PM
Unknown Object (File)
Tue, Nov 19, 9:22 PM
Unknown Object (File)
Thu, Nov 14, 3:07 AM
Unknown Object (File)
Oct 30 2024, 8:32 PM
Unknown Object (File)
Oct 29 2024, 10:40 PM
Unknown Object (File)
Oct 21 2024, 6:14 AM
Unknown Object (File)
Oct 18 2024, 7:20 AM
Unknown Object (File)
Oct 8 2024, 3:00 AM
Subscribers

Details

Summary

The legacy Stone algorithm uses int to represent line numbers, array
indices, and array lengths. If given inputs approaching INT_MAX lines,
it would overflow and attempt to allocate ridiculously large amounts of
memory. To avoid this without penalizing non-pathological inputs,
switch a few variables to size_t and add checks while and immediately
after reading both inputs.

MFC after: 3 days
PR: 280371
Sponsored by: Klara, Inc.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 58868
Build 55755: arc lint + arc unit

Event Timeline

des requested review of this revision.Jul 27 2024, 5:42 PM
allanjude added a subscriber: allanjude.
allanjude added inline comments.
usr.bin/diff/diffreg.c
558

Was it intentional to drop the RH_EOF case here?

This revision is now accepted and ready to land.Jul 27 2024, 6:00 PM
des marked an inline comment as done.Jul 27 2024, 6:03 PM
des added inline comments.
usr.bin/diff/diffreg.c
558

Yes, its only purpose was to silence a compiler warning.

des marked an inline comment as done.Jul 27 2024, 6:04 PM
yuri requested changes to this revision.Jul 27 2024, 6:57 PM
yuri added a subscriber: yuri.

Shouldn't

if (len[0] > INT_MAX - 2)

be

if (len[0] > SIZE_MAX - 2)

?

And shouldn't all int types representing file size be made 64-bit?
They should be uint64_t.

This revision now requires changes to proceed.Jul 27 2024, 6:57 PM

Shouldn't

if (len[0] > INT_MAX - 2)

be

if (len[0] > SIZE_MAX - 2)

?

No, because we need len[0] + 2 to not exceed INT_MAX.

And shouldn't all int types representing file size be made 64-bit?
They should be uint64_t.

The variables used to store file sizes are already size_t. The algorithm operates on line numbers, not byte offsets, and it uses negative values. I'm OK with refusing to handle files with more than 2^31 lines.

This revision is now accepted and ready to land.Jul 29 2024, 1:59 PM
This revision was automatically updated to reflect the committed changes.