https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77606
--- Comment #3 from Martin Sebor <msebor at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #2)
> The only bug is in the testcase, using __builtin_object_size (, 2) for this
> is just wrong
Unless the manual does a better job explaining things it's hard to guess how to
use the feature, even for someone who has been working with this feature for
years. Here's what I see there now:
The intended use can be e.g.
#undef memcpy
#define bos0(dest) __builtin_object_size (dest, 0)
#define memcpy(dest, src, n) \
__builtin___memcpy_chk (dest, src, n, bos0 (dest))
Given this example of intended use it seems that another such example is one
where the bos macro is defined like so:
#define bos2(dest) __builtin_object_size (dest, 2)
#define memcpy(dest, src, n) \
__builtin___memcpy_chk (dest, src, n, bos2 (dest))
If this isn't meant to be a valid example then the manual needs to be clarified
and explain what some of the other examples might be (and also what would not
be valid examples). I can appreciate that passing 0 as the object size to
__builtin___memcpy_chk and interpreting it as "unknown size" in the absence of
any other information is tricky. But that's a problem with (defect in) the
design of the builti-ins that needs to highlighted in the manual so users know
to avoid it. This was also one point of the test case in comment #0. My bad
for not articulating it more clearly.
> 0 is completely valid return value for that builtin, it is
> the "don't know" value that is returned whenever the code doesn't figure it
> out.
Exactly. Overloading the return value this way is the problem that either
needs to be solved or that needs to be documented in the manual (i.e., the
manual needs to explain that the use of the bos2() macro may lead to false
positives in some cases and give examples of some of them).
> __builtin_object_size (, 0) in this case also returns -1, "don't know" value.
>
> So, if anything, this can be treated as an enhancement request. Right now
> for malloc-like calls, including calls with alloc_size argument, we do:
> if (arg1 < 0 || arg1 >= (int)gimple_call_num_args (call)
> || TREE_CODE (gimple_call_arg (call, arg1)) != INTEGER_CST
> || (arg2 >= 0
> && (arg2 >= (int)gimple_call_num_args (call)
> || TREE_CODE (gimple_call_arg (call, arg2)) != INTEGER_CST)))
> return unknown[object_size_type];
> So, the enhancement request would be for gimple_call_arg (call, argN) here
> being an SSA_NAME instead of INTEGER_CST to try to prove some range instead
> (and for the multiplication case do it with extra care because of possible
> overflows).
I haven't studied the implementation in enough detail to comment on the
suggestion above but I can say what my expectation is from a user's point of
view: since there is no practical difference between an object allocated by
'__builtin_malloc (1 < argc ? 40 : 80)' and one allocated by '1 < argc ?
__builtin_malloc (40) : __builtin_malloc (80)', then since writing past the end
of one such object is detected so should be writing past the end of the other.
When it isn't, users don't care if it's due to some inherent limitation in the
GCC representation of the two expressions, or what changes in GCC might be
required to make it so. To them, the feature simply doesn't work the way they
expect and the way it's documented to. In most users' minds that's also not an
enhancement but a deficiency.
> I have reservations about using get_range_info for that though, the VRP
> stuff is computed from many sources, including assumptions that undefined
> behavior will not happen etc. But, these builtins are primarily used for
> security purposes, and the stuff it is guarding is also undefined behavior,
> so if anything, it should do some cheap short walk of the operands and
> compute range more conservatively (perhaps only allow SSA_NAME copies,
> perhaps casts, and PHIs with all constant arguments or something similar?).
I don't necessarily insist on using VRP here. If there's a different/better
way to do it I'm all for it. But if there isn't, I don't think that just
because VRP assumes the absence of undefined behavior it can't or shouldn't be
used to help detect its absence in calls to memcpy et al., unless it causes
false positives.
