FYI: Fix PR632: patches/security/20110215/6878713.patch breaks shark zero build

[Dr Andrew John Hughes]

On 10:02 Mon 21 Feb     , Mark Wielaard wrote:
> Hi Andrew,
> 
> Thanks for the constructive email. We were both a bit too over eager to
> get "our" builds to be fixed first/best. Thanks also for talking a
> little off list to make sure we were keeping constructive going forward.
> 

Any time.

> On Thu, 2011-02-17 at 21:02 +0000, Dr Andrew John Hughes wrote:
> > On 19:36 Thu 17 Feb     , Mark Wielaard wrote:
> > > No. I am happy to also test the default configuration, which I did, if I
> > > am fixing issues on a non-default configuration  that I care about. But
> > > I am not going to test all random variants that might happen.
> > > 
> > I'm not asking for 'all random variants'.  That would be kind of crazy and
> > almost unachievable :-)
> > 
> > I do ask that, when there is an alternate HotSpot build available, then
> > patches are checked against both.  While hs20 may not be the default now,
> > it will become so at some point in the future (probably after the imminent
> > 1.10 branch).  It will make things easier if it's not broken when we switch
> > over.  The HotSpot build option is fully documented in INSTALL.
> 
> That does take a lot of extra time though. I really do think we
> shouldn't force people to do multiple full builds, except for the
> default one and the variant that they are hacking/testing. 

Please bear in mind that I'm not asking this for all builds; just
those that change HotSpot.  JDK changes, for example, only need a
default build.

> It would be
> nice if we could somehow integrate this like we do with other
> alternative VMs with --enable-additional-vms. So you can easily build
> them during the main build, without having to rebuild everything else.
> And people might find it nice to try them both in their builds.
> 

As I think I've mentioned before, I'm very wary of the additional VMs option.
>From my perspective, there is already a way of building these VMs (--enable-shark,
--enable-cacao, --enable-zero) that ensures that the resulting VM is fully tested
by rebuilding OpenJDK using it.  It seems perfectly possible, with addvm, to produce
VMs that are untested and may not actually even run 'java -version'.

I can see how it would be advantageous from a speed perspective, but for me,
if I'm going to bother building one of the alternative VMs, I want to know it
actually works.  Now I guess you can run 'check-hotspot' with each of the alternative
VMs but I don't see why that saves anything over just doing the full build,
which tests the VM heavily with lots of javac invocations.

This isn't merely me wondering about its usage.  I recall times when Shark would
build for an addvm run but wasn't capable of completing the full bootstrap.

> Also, could we have an alias for "hotspot=next" or something? Assuming
> that we only support "current" and "next" hotspot, then you don't have
> to remember which particular version it represents (also easier for
> making sure a buildslave tracks them).
> 

I think what you want is already catered for; as documented in README,
a simple --with-hotspot-build will get you the alternative HotSpot.

  ORIGINAL_BUILD="original"
  ALTERNATE_BUILD="hs20"
...
   if test "x${HSBUILD}" = xyes; then
        HSBUILD="${ALTERNATE_BUILD}"
  elif test "x${HSBUILD}" = xno; then
        HSBUILD="${ORIGINAL_BUILD}"
  fi

so the two alternatives are always available via --with-hotspot-build
and --without-hotspot-build, the former being the one from the HotSpot
tarball, the latter from the original OpenJDK tarball.

> Xerxes offered to change one of his buildslaves to track hs20, so
> hopefully that will also help making sure that setup doesn't
> accidentally break.
> 

Awesome.

> > > If you care about non-default configurations please either make sure
> > > that it is in the default build/test setup. Or provide a buildslave for
> > > it, so that it is regularly tested on all commits. That is what Xerxes
> > > and I did for the non-default configurations that we care about (for
> > > x86_64 zero/shark isn't enabled by default, and not everybody has an arm
> > > setup).
> > 
> > I don't know how to do any of this.  I didn't see any discussion or list of
> > what was being added to these build bots.  If I missed it, I'm sorry.  I would
> > have suggested that both the alternate HotSpot build (currently hs20) be checked
> > and also icedtea6-hg if possible.  
> > 
> > The alternate HotSpot build and icedtea6-hg cover what's "coming soon"
> > to IcedTea6 (the HotSpot build from the new stable HotSpot tree,
> > icedtea6-hg from the upstream OpenJDK6 tree) and allows us to find
> > things out early rather than getting hit by it all in one big lump
> > when a new tarball is released.
> 
> Agreed. And sorry for not formally announcing the existence of the
> buildbot. Xerxes and I were still experimenting with various setups. And
> I admit I was a little ashamed of my hacky master.cfg setup. But I
> checked them all in and will send a more formal announcement with some
> instructions how anybody can add a buildslave or suggest changes in the
> current setup.
> 

Great.  That would be good.
Now I just need to find time to look at it ;-)

> > > > I'd prefer not to have yet more work on my plate because people don't do full
> > > > testing of patches and commit them without any review.
> > > 
> > > It is hard to get a good feeling for tone in an email, so apologies for
> > > misinterpreting. But I really don't appreciate this nitpicking. 
> > 
> > It wasn't meant to nitpick.  It was more a plea for help.  It's great
> > that you want to spend time looking into this and providing a fix, but
> > the help is negated if it means I have to spend time doing builds to
> > check for breakage or start changing things.  Effectively, it's
> > forcing me to look into the issue immediately, so if I sound a little
> > negative, that's why.
> 
> And also my apologies. I wasn't exactly nice in formulating my own
> attempts at "helping" and asking for help. We are all a little short on
> time.
> 

Thanks.

> > > You
> > > committed a patch, that you didn't discuss on the list, which broke the
> > > zero/shark configuration that Xerxes and I care about. 
> > 
> > In fairness, I can't really discuss embargoed security issues :-D
> 
> We probably should discuss that a bit more. It was unfair of me to treat
> these as if the were "normal" patches. You take these security issues
> on, and really do make sure they get applied as soon as possible, when
> the underlying issues are announced to the world at large. But we might
> need a bit more formal "security team" approach to make sure you don't
> get overwhelmed by them. Should we start a new thread on how to get more
> help with this process? I admit to not exactly know how you get into
> possession of these embargoed security fixes ahead of time, who
> embargoes them, what the process is if the happen to become public
> before the embargo date, or who else is involved, etc.
> 

You're right; we haven't documented this very well, at least publicly.

There are two sources of security patches; us and Oracle.  As a result,
there are two different ways in which they are dealt with.

With our own security patches (for NetX and the plugin), we have much more
choice in how to handle things.  We set an exact date and time that the
issue will be unembargoed, and prepare tarballs.  The work is fairly
distributed, as it's usually Omair and Deepak who discover such issues
and provide patches for the various versions of IcedTea.  I then do
the actual application of the patches, and create and build the release
tarballs.  We currently supply Fedora, Debian & Ubuntu (via doko) and
SuSE (via their security team) with encrypted tarballs ahead of time.
If any other distros want to be in this loop, they should contact me
privately with an encryption key.

With patches from Oracle, our heads are more tied.  We (as Red Hat)
receive the patches.  I'm not sure what the arrangement was for this,
as it was established prior to me joining the company.  I believe doko
also receives them from Oracle.  I then proceed to do the best I can
to apply these patches to the various IcedTea releases.  Backporting
to older releases is generally not the problem (actually having 1.7
with in-tree JAXP was an advantage this time round), getting all the
patches applying, building and not reproducing the issue is the main
issue.  The main issue, both this time and the last, was in getting a
decent tarball from Oracle.  It took three attempts on this occasion
with the complete set only becoming available on Friday morning.
That's why time was so limited.  With Oracle patches, we are merely
given a date and no time, so we have to continually watch for the
public announcement from them, which generally occurs in the late
evening or early hours of the following day here in Europe.  It would
be much easier if they supplied an exact time the patches could be
released.

I doubt more people would be much help.  It's pretty difficult to split
up the work of applying the patches (we've tried in the past).  As I say,
the main issue this time was patch availability, and thus out of our hands.
Also, generally, the fewer people know, the better in these cases.

Regarding issues that are already public, I guess we fix and release
ASAP.  With the floating point issue, again, Oracle's handling left a
lot to be desired.  I believe we could have had a release out with the
fix before FOSDEM, but we stalled in order to allow Oracle to handle
it.  And then they made a release of the fix with no prior warning,
and we had to play catch up.

> > No, I'll admit I didn't test Shark with the patches.  Having to do the
> > default and alternate HotSpot builds on three release branches and
> > HEAD three times over (due to us getting the security fixes in bits)
> > didn't leave much time for checking more estoric options.  With Shark,
> > in particular, I can't remember ever having had a successful build
> > more than a couple of times, so it's not high on my priorities.  I
> > understand it is with others, but then that's why I'm (hopefully)
> > not the only person working on IcedTea6 (though it does feel like
> > that sometimes).
> 
> You do take on a lot of the grunt work on making sure IcedTea keeps up
> to date and working nicely. Thanks. I admit that I was feeling like the
> only person caring about making sure the "alternative" runtimes kept
> working. Lets see if we can make this a bit smoother for all involved,
> whatever runtime/setup they care about. That is really why Xerxes and I
> started to work on the buildbot setup. See upcoming email.
> 

Well, it is my job I guess, but I would like to do less of it and work
on more interesting stuff as well if possible.  I look forward to the
buildbot helping out, as (contrary to the impression this thread may
give) I'm strongly in favour of getting patches in the tree and
available for public testing, rather than doing all of that locally.

> > Well, thanks for doing this (if a little belated).  It is appreciated.
> > I did see the issue on IRC but I've not had time to look at it.  To be
> > honest, the thanks would have been immediately forthcoming if you'd
> > just given me or someone else chance to scan over the patch first
> > before committing it.  That's not really for any technical reason so
> > much as it turns a "oh heck what's this coming down the line"
> > (assuming the worst, as here) into "Oh, great you fixed that.
> > Thanks".
> 
> Yes, felt it was discussed at length through the bug report and that it
> was "urgent" to fix asap, seeing that the autobuilders were all broken
> (because the only ones having been setup do always include all
> alternative runtimes). I'll add a more basic/plain build config to one
> of the buildslaves soon. And will try to have a bit more patience.
> 
> > I'll admit I'm not too good at doing that myself, but that's more born
> > from the experience that if I do, sadly no-one looks at it until I go
> > and find someone and prod them...
> 
> Grin. Well, to be honest, that was how I was feeling. Lets agree to be
> more aggressive/speedy in reviewing other people patches.
> 

Sometimes it's all I find myself doing :-)

> > I hope that clears the air a bit.
> 
> Yes, it did. Appreciated. Hope my reply do the same.
> 

Yes, it did.  Thanks.

> Thanks,
> 
> Mark
> 

Thanks,
-- 
Andrew :)

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

Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37