Post install script has error in RegEx

Bug #1666570 reported by Andy Wagner on 2017-02-21
20
This bug affects 2 people
Affects Status Importance Assigned to Milestone
tomcat7 (Debian)
New
Unknown
tomcat7 (Ubuntu)
High
Unassigned
Trusty
High
Joshua Powers
Xenial
High
Joshua Powers
Yakkety
High
Joshua Powers
Zesty
High
Unassigned
tomcat8 (Ubuntu)
High
Joshua Powers
Xenial
High
Joshua Powers
Yakkety
High
Joshua Powers
Zesty
High
Joshua Powers

Bug Description

== Begin SRU Template ==
[Impact]

 * On upgrade of tomcat7 package, if a user has updated their JAVA_OPTS variable to include a '%' an upgrade will fail. The sed command in the postinst uses the '%' character to act as a delimiter, previous versions used '/' however it was updated to '%' in hopes it was far less common.
 * This SRU updates it to a character that should not be found in the JAVA_ARGS value, namely '\001'.
 * This is the same solution Debian and tomcat maintainers are now using for Tomcat8.

[Test Case]

An example to test Tomcat7 on Trusty. The same instructions can apply to Tomcat8 on the other releases.

Overview: Install the version from the current release. Modify JAVA_OPTS and then install the version from proposed to validate it upgrades successfully.

 * lxc launch ubuntu-daily:trusty trusty
 * lxc exec trusty bash
 * apt install tomcat7
 * Edit /etc/default/tomcat7, set JAVA_OPTS="-Djava.awt.headless=true -XX:ErrorFile=/var/log/tomcat7/java_error%p.log -XX:+DisableExplicitGC -XX:+UseG1GC"
 * # Enable proposed
 * apt update
 * apt install tomcat7
 * When asked, 'Keep the local version currently installed'
 * With the fix, install will complete
 * Without the fix, the error as described under "Other Info" will appear

[Regression Potential]

 * Users currently experiencing this issue would be expecting a SRU fix to come from us. Working around it would require changing their JAVA_OPTS temporarily, accepting the maintainers version of the defaults script, or modifying the package's postinst script directly.
* In either case the proposed fix will over write any changes an end user may have made to the postinst, and all for correctly working expected behavior.
 * There is the slight, albeit incredibly low chance, that someone actually has the '\001' character in their JAVA_OPTS. In which, case the upgrade would fail as this bug describes.

[Other Info]

 * Using a new delimiter that is far less likely to be in someone's path. This is not the first time the delimiter has changed, as it originally as '/' which is obviously going to show up as soon as someone adds a path.
 * Upstream change to tomcat8: https://anonscm.debian.org/cgit/pkg-java/tomcat8.git/patch/?id=7664221d66701e2c31a31fe3b4f22e8bea4158dc
 * Error message on failure:

Setting up tomcat7 (7.0.52-1ubuntu0.10) ...
sed: -e expression #1, char 97: unknown option to `s'
dpkg: error processing package tomcat7 (--configure):
 subprocess installed post-installation script returned error exit status 1
Errors were encountered while processing:
 tomcat7
E: Sub-process /usr/bin/dpkg returned an error code (1)

== End SRU Template ==

Andy Wagner (rarian) on 2017-02-21
description: updated
Joshua Powers (powersj) wrote :

Thank you for taking the time to file a bug report and try to solve it!

Looking at your proposed resolution if we were to escape the $ then it
would replace %JAVA_OPTS with $JAVA_OPTS, however I do not believe that
is what we want. The goal of this line is to set the JAVA_OPTS based on
the following:

db_get tomcat7/javaopts && JAVA_OPTS="$RET" || JAVA_OPTS="-Djava.awt.headless=true -Xmx128m -XX:+UseConcMarkSweepGC"

You will also note how the previous two lines do not escape
$TOMCAT7_USER or $TOMCAT7_GROUP.

My initial guess is that JAVA_OPTS may have the '%' character in it.
That would in turn mess up the sed command since '%' is used as the
delimiter in that statement. In fact there was a similar Debian bug [1]
reported about this issue with the '/' character. There is a comment
as to changing to '%' as it shows up less often :)

[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=597814

Joshua Powers (powersj) wrote :

Can you get the output of the following:

$ cat /etc/default/tomcat7 | grep -i java_opts
$ debconf-show tomcat7

Changed in tomcat7 (Ubuntu):
status: New → Incomplete
Andy Wagner (rarian) wrote :

You are indeed correct, the culprit is a `%` in the java options of our affected servers, one example being:
JAVA_OPTS="-Djava.awt.headless=true -XX:ErrorFile=/var/log/tomcat7/java_error%p.log -Xms15025m -Xmx15025m -XX:+DisableExplicitGC -XX:+UseG1GC -XX:+UseStringDeduplication"

And for completeness:
$ debconf-show tomcat7
  tomcat7/groupname: tomcat7
  tomcat7/javaopts: -Djava.awt.headless=true -XX:ErrorFile=/var/log/tomcat7/java_error%p.log -Xms15025m -Xmx15025m -XX:+DisableExplicitGC -XX:+UseG1GC -XX:+UseStringDeduplication
  tomcat7/username: tomcat7

At first blush our solution worked, but this was a side affect of our configuration management system rewriting the file and camouflaging the fact that it isn't actually a solution at all!

Anyway, your explanation of what the regular expression is actually trying to accomplish leads to what I believe is a relatively trivial use of parameter expansion to escape the '%' in the JAVA_OPT environment variable:
| sed "s%^JAVA_OPTS=.*$%JAVA_OPTS=\"${JAVA_OPTS/\%/\\\%}\"%"

I was rather suspicious that this wasn't a known bug already, so I guess I should have dug deeper originally.

Andy Wagner (rarian) wrote :

Let me know if there is anything further I can do to move this forward.

Joshua Powers (powersj) wrote :

Thanks for the ping.

I've filed the bug with Debian. I'd like to first see what they think of my solution and then can get this SRU'ed back to Trusty.

Changed in tomcat7 (Ubuntu):
status: Incomplete → In Progress
assignee: nobody → Joshua Powers (powersj)
importance: Undecided → High
Changed in tomcat7 (Debian):
status: Unknown → New
Joshua Powers (powersj) wrote :

This is how upstream has fixed this:

https://anonscm.debian.org/cgit/pkg-java/tomcat8.git/commit/?id=7664221

Let me see about getting an SRU to trusty now.

Andy Wagner (rarian) wrote :

That is an elegant solution, thanks for staying on top of this!

Joshua Powers (powersj) wrote :

Created PPA with fix for Trusty here:
https://launchpad.net/~powersj/+archive/ubuntu/tomcat7-1666570

Attaching debdiff for trusty.

description: updated

The attachment "debdiff for trusty" seems to be a debdiff. The ubuntu-sponsors team has been subscribed to the bug report so that they can review and hopefully sponsor the debdiff. If the attachment isn't a patch, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are member of the ~ubuntu-sponsors, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issue please contact him.]

tags: added: patch
Joshua Powers (powersj) wrote :

Only source package, binary is not present in zesty

no longer affects: tomcat8 (Ubuntu Trusty)
Changed in tomcat7 (Ubuntu Zesty):
assignee: Joshua Powers (powersj) → nobody
status: In Progress → Invalid
Joshua Powers (powersj) wrote :

@Andy I have committed fixes for all changes to our importer. They will be reviewed and accepted, unless I need to make any changes :) Once accepted, the new packages will enter the proposed pocket for final testing and acceptance and after a minimum of 7 days released. I believe I have that right, but open to correction from anyone ;)

Changed in tomcat7 (Ubuntu Trusty):
assignee: nobody → Joshua Powers (powersj)
importance: Undecided → High
status: New → Fix Committed
Changed in tomcat7 (Ubuntu Xenial):
assignee: nobody → Joshua Powers (powersj)
importance: Undecided → High
status: New → Fix Committed
Changed in tomcat7 (Ubuntu Yakkety):
assignee: nobody → Joshua Powers (powersj)
importance: Undecided → High
status: New → Fix Committed
Changed in tomcat8 (Ubuntu Xenial):
assignee: nobody → Joshua Powers (powersj)
importance: Undecided → High
status: New → Fix Committed
Changed in tomcat8 (Ubuntu Yakkety):
assignee: nobody → Joshua Powers (powersj)
importance: Undecided → High
status: New → Fix Committed
Changed in tomcat8 (Ubuntu Zesty):
assignee: nobody → Joshua Powers (powersj)
importance: Undecided → High
status: New → Fix Committed
Joshua Powers (powersj) on 2017-03-09
description: updated

Hello Andy, or anyone else affected,

Accepted tomcat7 into yakkety-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/tomcat7/7.0.72-1ubuntu0.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed.Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

tags: added: verification-needed
Brian Murray (brian-murray) wrote :

Hello Andy, or anyone else affected,

Accepted tomcat7 into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/tomcat7/7.0.68-1ubuntu0.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed.Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Brian Murray (brian-murray) wrote :

Hello Andy, or anyone else affected,

Accepted tomcat7 into trusty-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/tomcat7/7.0.52-1ubuntu0.11 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed.Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Robie Basak (racb) wrote :

I've reviewed and uploaded Josh's MPs for tomcat8. Since there are three and some review comments are shared, I'm writing them up here to avoid confusion between the multiple MPs.

For all three, I had to s/tomcat7/tomcat8/ in the conffile pathname, as this has switched in tomcat8.

For Zesty, I changed the version from 8.0.38-2ubuntu1.1 to 8.0.38-2ubuntu2. 1.1 would work, but convention for the development release is that we would use ubuntu2. Exceptionally we are in final freeze so it may become an SRU anyway.

I missed that tomcat8 Zesty wasn't fixed before. I would have required this before uploading tomcat7 too had I noticed, because really the SRU rule of "fix development first" should apply in spirit between tomcat7 and tomcat8 because it is only a technicality that they have different source package names in this case. But as we're including Zesty in this upload, we're fixing that up anyway, and the tomcat7 fixes haven't hit the release pocket yet so users of not -proposed won't see a regression.

There was a bug in the "usd import" tool that caused Josh's Yakkety fixes to be based on 8.0.37-1 instead of 8.0.37-1ubuntu0.1 that is in yakkety-security currently. So the upload got rejected, I noticed, and rebased Josh's changes on top of the yakkety-security tree for now. The yakkety-devel branch pointer in the git imported tomcat8 repository is still wrong, but I think it may fix itself when it imports this upload. The commit hash will still be "wrong", but will be fixed when we re-import.

Release and SRU teams: as the SRU for tomcat7 is already in progress (due to my previous review omission), I figured that having this in the Zesty unapproved queue would be the best thing to do for now. It's a minor fix so we can skip it for Zesty release if you like, but we should process it as post-release SRU in Zesty as Josh is SRUing all the other releases anyway and Zesty would be left out otherwise.

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package tomcat8 - 8.0.38-2ubuntu2

---------------
tomcat8 (8.0.38-2ubuntu2) zesty; urgency=medium

  * Fix an upgrade error when JAVA_OPTS in /etc/default/tomcat8
    contains the '%' character (LP: #1666570).

 -- Joshua Powers <email address hidden> Tue, 28 Mar 2017 16:47:32 -0700

Changed in tomcat8 (Ubuntu Zesty):
status: Fix Committed → Fix Released
Robie Basak (racb) wrote :

I think everything is in -proposed or in the unapproved queues pending SRU review now, so unsubscribing ~ubuntu-sponsors (I forgot to do this when I uploaded).

Joshua Powers (powersj) on 2017-04-19
description: updated
Joshua Powers (powersj) wrote :

@sru-team

Verified tomcat7 trusty (7.0.52-1ubuntu0.11) based on the test case described in the SRU template. The update to the proposed version completed without error. Marking as verification-done-trusty.

I will do Xenial and Yakkety once I have binaries.

tags: added: verification-done-trusty
Joshua Powers (powersj) wrote :

@racb,

It looks like the uploads to Xenial and Yakkety did not produce binaries. Looking at the logs the builds failed on tests. I uploaded the current released version in xenial to a ppa and it also failed. What are my next steps?

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.