RFR: 8u: 8076475: Misuses of strncpy/strncat

[Andrew Hughes]

On 26/05/2020 09:57, Andrew Dinn wrote:
> Hi Andrew(s)
> 
> On 25/05/2020 07:16, Andrew Hughes wrote:
>>
>>
>> On 03/04/2020 10:18, Andrew Haley wrote:
>   . . .
>>> Here is my reasoning. The need for stability and the minimization of
>>> churn increases over time. People use JDK 8 because they want that
>>> stability, and every patch carries some risk. So, ecause it is now a
>>> mature piece of software, patches should be minimal where they
>>> reasonably can. The practice of, by default, pulling in dependencies
>>> from later releases is too risky: they might well be broken in ways
>>> that the old version wasn't, and they may depend on other changes in
>>> the recent code base.
>>>
>>> Now, that doesn't mean that such dependent changesets must *never* be
>>> pulled in, especially when they are needed by several backports. But
>>> if dependency changesets substantially multiply the size of a change,
>>> as they do in this case, this should not be done.
>>>
>>> I am aware that this does not ease the life of the maintainers, but
>>> that isn't the goal. Stability is.
>>
>> It's not about making life easier for maintainers; reviewing and
>> approving more patches means the opposite. My concern is that the more
>> 8u diverges from later versions, and uses its own bespoke version of
>> fixes, the greater the knowledge and experience needed for someone to
>> work on it. We're already struggling to keep up as it is.
> 
> Well, that's one direction of travel. This is, however, a two way
> street. The more jdk8u pulls in changes from later versions instead of
> using its own bespoke version of fixes, the more likely it is that one
> of those changes is going to be based on assumptions baked into the
> upstream repo code that clash with assumptions baked into jdk8u repo
> code. Understanding why something is breaking the jdk8u rules and how
> that can be squared with what the the rest of the jdk8u code does is an
> equal peril.
> 
> The danger is subtle errors where we fall foul of races, corner cases,
> re-orderings of control or synchronization, different interpretation of
> the same values in the same flags, counters or object fields. The
> problems that may arise in this way cannot really easily be avoided by
> adapting a patch as it is backported, even less so by pulling in more
> patches. The obvious safeguards that are provided by getting the code to
> compile and pass tests won't remove this sort of risk. The only way to
> avoid those errors as you pull in ever more changes is to backport
> everything so that the design decisions that constrain how jdk8u
> operates have all been replaced with the upstream ones. However, that's
> clearly not the destination we need to aim for.
> 
> None of these complex, subtle risks can be avoided via a simple,
> mechanical process. All of them require the application of careful
> judgement and a deep knowledge of the code bases involved. The more
> releases there are between the original version of a patch and it's
> application downstream the more thinking there needs to be. Given enough
> distance (and with many patches we have more than enough distance even
> between jdk11u and jdk8u) we can no longer afford to proceed by tweaking
> an upstream patch, even less so by pulling in all the apparatus it
> depends on. We have to think about the bug (or perf issue), think about
> what that bug means in the context of jdk8u and then think about
> designing a patch that fixes it from the point of view of jdk8u. The
> original patch /may/ do the job, or /may/ guide us to the right fix.
> However, if the upstream patch is dependent on a lot of other changes
> not present in jdk8u then that's a flashing red light saying that this
> patch won't do and that a custom patch is a better, safer bet.
> 
>> Each case has to be considered separately, according to risk and the
>> changes involved. It's not a guarantee of stability to just not make
>> changes. If it leads to the changes that are made to 8u being unique to
>> that codebase, and not as widely tested as something that has been in
>> later versions for years, we will introduce more 8u-specific bugs. In
>> short, there is no easy answer and no one size fits all solution.
> 
> Yes, that is right. However, I agree with Andrew that we need to have a
> very healthy mistrust of any patch that requires us to pull other
> changes in its wake. Upstream fixes may legitimately depend on other
> upstream fixes because ... well, they were already there and, when
> added, one can assume they were known to be necessary and safe in that
> upstream context. Downstream fixes should normally only presume the
> presence of such a dependency if there is a compelling need to have that
> dependency /in its own right/. If not then the default assumption has to
> be that a downstream fix should be sought without relying on that
> upstream dependency. Pulling in multiple patches simply because that was
> a legitimate upstream choice is a very slippery and dangerous slope.
> 
> regards,
> 
> 
> Andrew Dinn
> -----------
> 

Completely agree. My aim was to balance what aph said, rather than
suggest that we go to the other extreme. I also think we need to be very
aware of the growing expertise needed to maintain 8u. Thank you for
expanding on this in more detail.

My experience of working on backports for 6u & 7u over the last decade
or so suggests that no situation is ever the same as another. In one
case, you may need to completely rewrite the fix (common with HotSpot,
where there are often significant refactorings and major reworkings). In
another, it may make sense to backport a dependent change, because it's
low risk and very likely to be a dependency of other future changes
(common with testing infrastructure)

When I look a a proposed backport, I like to see that potential
dependencies have been considered, even if it is then decided to work
around them. My concern with the initial version of this patch was that
it lifted a function from another fix, but without the accompanying
changes that made use of that function. Thus, we ended up with a case
where 8u had a mix of behaviour; the new cases in the backported patch
would use the new function, while older cases didn't (in contrast to
later OpenJDK versions). The final version we pushed backports less of
the original patch and I feel is better for it.

To end on a positive note, I'm glad to say we tend to have more
information available to make these decisions these days than we have in
the past, thanks to greater transparency and in particular, the
vulnerability group. In the past, we have had to go the route of
backporting more than would be desirable, simply because it is the
safest way to ensure an issue is fixed, when you have little to no
knowledge of the original problem.

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222