On 10/22/18 9:03 PM, Martin Sebor wrote: > On 10/22/2018 09:08 AM, Bernd Edlinger wrote: >> Hi! >> >> This makes c_strlen avoid an unsafe strlen folding of const arguments >> with non-const offset. Currently a negative out of bounds offset >> makes the strlen function return an extremely large number, and >> at the same time, prevents the VRP machinery, to determine the correct >> range if the strlen function in this case. >> >> Fixed by doing the whole computation in size_t and casting the >> result back to ssize_t. >> >> >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. >> Is it OK for trunk? > > In general, I view folding to a "safe" value as done in this patch > (and as Richard suggests in his comment on the bug) far preferable > -- as in far safer/more secure -- to avoiding folding in these > cases (currently the prevailing strategy). >
Well, in this case we have the slightly different situation, where 'strlen(string_cst + x)' is folded to 'x <= len ? len - x : 0'. The string_cst does not have embedded nuls, and may be declared like 'const char string_cst[100] = "string"'. So the COND_EXPR is not there to separate valid from invalid inputs, but instead we may assume that x does never exceed the array bounds. However it still results in more accurate range infos, to do all computations in unsigned arithmetics, and a slightly more safe result if the x is negative at the same time, so win-win. But I have no doubt that it would be even more safe to have a trap if x is out of bounds, however if we add another conditional branch, that would be something that needs to be optional, and results in slightly less efficient code. > If there is consensus on adopting this approach for strlen I'd like > to see it applied in other such cases as well as a matter of policy. > > Some other similar examples to consider include: > > * calling other built-ins on such arrays, including strnlen, > memchr/strchr, and strpbrk > * indexing into such an array (note that accesses at constant > out-of-bounds indices into constant arrays are already folded > to zero, although that may be an accidental rather than > a conscious decision made to avoid the worst kind of fallout). > I would argue that if the undefinedness of any such construct can be determined statically, there should be a warning, and the folding should not be done, the out-of-bounds index handling is also something that I would remove eventually. The folding should not be done when it is evident that the index exceeds the memory size, as returned by string_constant, while it is still okay to fold to 0, if the index is somewhere in between the string length and the memory size. That can be determined statically, and comes at no extra cost. Bernd.
