Bug #10785

_get_tg_setting() broken with set -u

Added by anonym 2015-12-21 11:54:09 . Updated 2016-01-27 13:31:55 .

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Persistence
Target version:
Tails_2.0
Start date:
2015-12-21
Due date:
% Done:

100%

Feature Branch:
bugfix/10785-_get_tg_setting-vs-set-u
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Deliverable for:

Description

I saw this in the journal:

Dec 21 15:53:40 amnesia add-GNOME-bookmarks[3543]: /usr/local/lib/add-GNOME-bookmarks: 1: eval: TAILS_PERSISTENCE_READONLY: parameter not set


Indeed, tails-greeter.sh’s _get_tg_setting() (via persistence_is_enabled_read_write()) is not adapted for set -u.

Subtasks

History

#1 Updated by anonym 2015-12-21 11:56:30

Hmm. I would expect add-GNOME-bookmarks to fail to create the bookmark for ~/Persistent/Tor Browser, but it does work. Any explanation?

I still think we should fix this…

#2 Updated by anonym 2015-12-21 11:57:42

  • Status changed from Confirmed to In Progress

Applied in changeset commit:2d28e3143593c5ce33f6e34a005a389e10586adb.

#3 Updated by anonym 2015-12-21 11:58:09

  • Assignee deleted (anonym)
  • % Done changed from 0 to 50
  • QA Check set to Ready for QA
  • Feature Branch set to bugfix/10785-_get_tg_setting-vs-set-u

#4 Updated by anonym 2015-12-21 12:00:17

Just noticed that create-tor-browser-directories is “addected” by the same issue, which this branch also will fix.

#5 Updated by intrigeri 2015-12-22 02:31:10

  • Assignee set to intrigeri

Thanks!

#6 Updated by intrigeri 2016-01-02 04:17:11

(Removed buggy explanation, I misread the question.)

#7 Updated by intrigeri 2016-01-02 04:26:01

anonym wrote:
> Hmm. I would expect add-GNOME-bookmarks to fail to create the bookmark for ~/Persistent/Tor Browser, but it does work. Any explanation?

The buggy statement is eval’ed by a function that’s itself called in a subshell with $(). Looks like set -e is lost somewhere along the way. Did I mention that I find it hard to write robust shell scripts? :)

#8 Updated by intrigeri 2016-01-02 08:00:31

  • Assignee changed from intrigeri to anonym
  • QA Check changed from Ready for QA to Dev Needed

I have merged this branch locally, built an ISO, upgraded a USB stick with it, booted, and I see the same message in my logs. So apparently the branch does not really address the problem. Am I doing it wrong, or?

#9 Updated by anonym 2016-01-02 22:32:02

  • Assignee changed from anonym to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> The buggy statement is eval’ed by a function that’s itself called in a subshell with $(). Looks like set -e is lost somewhere along the way.

No, set -e is not lost. Note that set -e will only make the current scope return with an error, so execution will only abort on the top level of the script. So the buggy reference of the undefined variable in _get_tg_setting() will just make it return false thanks to set -u + set -e, and then set -e will trigger in the caller, persistence_is_enabled_read_write(), making it return false, which is an expected output, but now it means something completely different. Yay. In a better language we’d have an exception being raised here instead of overloading the boolean value “false” with “execution error”.

> Did I mention that I find it hard to write robust shell scripts? :)

I could not agree more! :)

> I have merged this branch locally, built an ISO, upgraded a USB stick with it, booted, and I see the same message in my logs. So apparently the branch does not really address the problem. Am I doing it wrong, or?

I have force pushed the fix. I believe I originally fixed the script in a live session and tested it successfully, but then transcribed the code incorrectly into Tails’ Git. Sorry!

#10 Updated by intrigeri 2016-01-02 23:11:26

> No, set -e is not lost. Note that set -e will only make the current scope return
> with an error, so execution will only abort on the top level of the script. So the
> buggy reference of the undefined variable in _get_tg_setting() will just make it
> return false thanks to set -u + set -e, and then set -e will trigger in the
> caller, persistence_is_enabled_read_write(), making it return false, which is an
> expected output, but now it means something completely different. Yay. In a better
> language we’d have an exception being raised here instead of overloading the boolean
> value “false” with “execution error”.

O.M.G. /me, fleeing.

> I have force pushed the fix.

Thanks! I’ll look at it shortly.

Glad to see you back, by the way! :)

#11 Updated by intrigeri 2016-01-03 12:09:03

  • Status changed from In Progress to Fix committed
  • % Done changed from 50 to 100

Applied in changeset commit:563b18c6e1fe85bb481b63c4ac56bb01d5444d2c.

#12 Updated by intrigeri 2016-01-03 12:10:52

  • Assignee deleted (intrigeri)
  • QA Check changed from Ready for QA to Pass

#13 Updated by anonym 2016-01-27 13:31:55

  • Status changed from Fix committed to Resolved