Slight glitch in /etc/cron.daily/apt-compat Ubuntu 16.04.3 in apt-1.2.24

Bug #1742378 reported by rklemme
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
apt (Ubuntu)
Low
Julian Andres Klode

Bug Description

I accidentally stumbled across this. If in line 22 on_ac_power returns a non zero exit code the next line is never reached. Instead, the shell terminates immediately with that non zero exit code because of "set -e" in line 3. In this script it does not pose a problem because the shell is asked to exit anyway if check_power() returns non zero but it works differently than is apparently intended (concluding from the logic).

I will attach a suggest patch and a demo illustrating that the patch works as the original script seems to intend to.

Revision history for this message
rklemme (shortcutter) wrote :
Revision history for this message
rklemme (shortcutter) wrote :

Produces this output:

$ sh check_power_test.sh
+ echo begin
begin
+ check_power 0
+ exit 0
+ return 0
+ check_power 1
+ exit 1
+ [ 1 -ne 1 ]
+ return 1
+ echo ignore
ignore
+ check_power 255
+ exit 255
+ [ 255 -ne 1 ]
+ return 0
+ echo end
end

Revision history for this message
rklemme (shortcutter) wrote :

I forgot to mention one thing about the patch: the current behavior is that when on_ac_power returns 255 the current version of the script returns with that non zero error code and does not execute /usr/lib/apt/apt.systemd.daily. The patched version will execute /usr/lib/apt/apt.systemd.daily and terminate with whatever exit code that process generates.

On a side note, "which" can be replaced by "type" which is POSIX compatible and a shell built in.

Revision history for this message
Julian Andres Klode (juliank) wrote :

I'd rather use if ! on_ac_power; then return 1; fi

Changed in apt (Ubuntu):
importance: Undecided → Low
status: New → Triaged
assignee: nobody → Julian Andres Klode (juliank)
Revision history for this message
Julian Andres Klode (juliank) wrote :

or well, use an explicit if

Revision history for this message
Julian Andres Klode (juliank) wrote :
Changed in apt (Ubuntu):
status: Triaged → Fix Committed
Revision history for this message
rklemme (shortcutter) wrote :

@Julian I had considered that. This could be more easily done with "on_ac_power || return 1" - there is no need for full "if...then...fi".

But both those solutions would have different semantics than what the script _apparently_ intends to do: on_ac_power returns 0 on AC, 1 on battery and 255 if the power status cannot be determined. The current code of the script indicates that in this case /usr/lib/apt/apt.systemd.daily should be executed and _only_ be omitted if on_ac_power returns exactly 1.

Revision history for this message
Ubuntu Foundations Team Bug Bot (crichton) wrote :

The attachment "Suggested patch to fix" seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

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

tags: added: patch
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package apt - 1.6~alpha7ubuntu1

---------------
apt (1.6~alpha7ubuntu1) bionic; urgency=medium

  * Try to work around test-method-mirror failure by setting umask at start

 -- Julian Andres Klode <email address hidden> Wed, 31 Jan 2018 12:19:58 +0100

Changed in apt (Ubuntu):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers