Page MenuHomeFreeBSD

tmpfs: Fix comparison in tmpfs_extattr_update_mem()
Needs ReviewPublic

Authored by markj on Tue, Dec 16, 5:23 PM.
Tags
None
Referenced Files
F139858384: D54258.id168203.diff
Wed, Dec 17, 4:55 AM
F139858340: D54258.id.diff
Wed, Dec 17, 4:54 AM
F139853681: D54258.id168203.diff
Wed, Dec 17, 3:17 AM
F139853461: D54258.id.diff
Wed, Dec 17, 3:13 AM
F139852596: D54258.diff
Wed, Dec 17, 2:54 AM
Subscribers

Details

Reviewers
fsu
kib
Summary

The tm_ea_memory_inuse and tm_ea_memory_max fields are unsigned, so the
signed "size" is promoted to unsigned before the comparison. When
"size" is negative, this will not work as expected.

Reported by: Kevin Day <kevin@your.org>
Fixes: 56242a4c6566 ("Add extended attributes")

Diff Detail

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

Event Timeline

markj requested review of this revision.Tue, Dec 16, 5:23 PM

This is only relevant for 32bit arches, am I right? (I do not claim that it is not a bug to fix).

In D54258#1239826, @kib wrote:

This is only relevant for 32bit arches, am I right? (I do not claim that it is not a bug to fix).

Hmm, now that I look at it again, I think this patch is just bogus, and there is no problem. If size < 0 and size is promoted to an unsigned type, addition with tm_ea_memory_inuse will cause wraparound and the result will be correct. It does not make much sense to perform the comparison at all if size < 0, but I think there is no problem, even on 32-bit arches. I did not test it at all yet.

In D54258#1239826, @kib wrote:

This is only relevant for 32bit arches, am I right? (I do not claim that it is not a bug to fix).

Hmm, now that I look at it again, I think this patch is just bogus, and there is no problem. If size < 0 and size is promoted to an unsigned type, addition with tm_ea_memory_inuse will cause wraparound and the result will be correct. It does not make much sense to perform the comparison at all if size < 0, but I think there is no problem, even on 32-bit arches. I did not test it at all yet.

I mean, on 64bit ssize_t is same sizeof() as ea_memory_inuse, so promotion to unsigned then results in the expected wraparound. On 32bit, size is 32bit, and my wrong thought was that unsigned promotion would be to uint32_t, but really it promotes to uint64_t and properly propagates sign into the upper word. So indeed, the change should be not needed.