Bug 235589

Summary: sh(1): LINENO is unset in shell arithmetic
Product: Base System Reporter: Martijn Dekker <mcdutchie>
Component: binAssignee: Jilles Tjoelker <jilles>
Status: Open ---    
Severity: Affects Some People CC: bugs, jilles, me
Priority: --- Keywords: needs-qa
Version: 12.0-STABLEFlags: koobs: mfc-stable12?
koobs: mfc-stable11?
Hardware: Any   
OS: Any   
Attachments:
Description Flags
Proposed fix patch
none
A possible fix
none
Perfect LINENO behavior? none

Description Martijn Dekker 2019-02-07 23:01:32 UTC 
In sh(1), the LINENO variable (currently executed line number) is always zero when it is read as part of a shell arithmetic expression.

For instance:

$ echo $LINENO $((LINENO)) $(($LINENO))
108 0 108

Expected: 3 times "108" (or whatever the current line number is)

Note that $(($LINENO)) does work because it expands LINENO as a normal shell expansion before invoking the arithmetic subsystem. However, $((LINENO)) should be equivalent as it is for all other variables.

Another manifestation of the bug is:

$ set -u
$ echo $((LINENO))
-sh: arithmetic expression: variable not set: "LINENO"
Comment 1 Paco Pascal 2019-08-02 02:42:15 UTC 
Created attachment 206211 [details]
Proposed fix patch

This is my first attempt at submitting a patch to FreeBSD. I'm not sure if it's good practice to submit a potential patch directly to the bug report or to first have it reviewed elsewhere such as the mailing lists.
Comment 2 Kubilay Kocak freebsd_committer freebsd_triage 2019-09-05 11:32:06 UTC 
(In reply to Paco Pascal from comment #1)

Proposing patches is always welcome with or without having it discussed or reviewed prior. Having said that, it can and does help, usually to improve the proposed patch, if one can get some feedback on it.

@Jilles (CC'd), with experience in this area of the tree, may know what to do with this
Comment 3 Jilles Tjoelker freebsd_committer freebsd_triage 2019-09-08 21:29:42 UTC 
I like that this does not slow down the main execution loop much, since it only does the conversion from integer to string when LINENO is referenced.

This part could be slightly less magic by making a fixed struct var for LINENO (like HISTSIZE, IFS, MAIL, MAILPATH, etc.). This fixed struct can also point to a sufficiently large buffer "LINENO=1\0\0\0\0\0\0\0\0\0\0".

I would like to see this new mechanism used instead of the old VSLINENO mechanism (so there aren't two separate mechanisms for $LINENO and $((LINENO))).

Given that this will subtly change LINENO values anyway (try expanding LINENO twice in different lines of a word spanning multiple lines), I think it would make more sense to make it more like other shells do, reading the line number at the start of a command containing expansions (NCMD/NCASE/NFOR).

With this, it should be possible to set PS4='$LINENO+ ' and see the script's line numbers in set -x output.

You can compare https://git.kernel.org/pub/scm/utils/dash/dash.git/commit/?id=5bb39bb1995cb12d8da76b1d482df9be1acc2eb4 which is a similar implementation for dash (another Almquist variant) but I don't think there will be much benefit in copying code from there.
Comment 4 Paco Pascal 2020-02-29 18:53:23 UTC 
Created attachment 212048 [details]
A possible fix
Comment 5 Paco Pascal 2020-03-01 01:38:11 UTC 
Created attachment 212062 [details]
Perfect LINENO behavior?

This patch is the same as the last one I posted, except it asserts that LINENO can't be 0 and LINENO in functions were off by 1. In the previous patch I posted,

    echo `
    echo $LINENO`

would output 0. There might be a better method of ensuring this than what I did in parser.c.

As far as I can see, this makes LINENO behave in every way it should. The following,

    echo $LINENO $((LINENO)) $(($LINENO))
    PS4='$LINENO+ '
    echo `echo $LINENO`
    eval 'echo $LINENO $((LINENO)) $(($LINENO))'

all perform in what I think is expected ways.
Comment 6 Jilles Tjoelker freebsd_committer freebsd_triage 2020-03-01 23:06:26 UTC 
This is quite nice, but please run the tests (basically, cd /usr/tests/bin/sh && kyua test) and add new tests for $((LINENO)). Existing tests should not be modified unless they were broken or too specific.

I think NCASE and NFOR nodes should also contain a line number.

An earlier version of the patch did a simple 32-bit integer write during execution, leaving the snprintf(..., "%d", ...) to when LINENO was expanded. I liked that better. There could be a VLINENO flag indicating LINENO's struct var (and that it is still special, see below).

Making LINENO readonly matches zsh but I found some obscure scripts searching for "LINENO=" on codesearch.debian.net (such as https://sources.debian.org/src/opentmpfiles/0.2+2019.05.21.git.44a55796ba-2/tmpfiles.sh/?hl=443#L443 ) that assign values to LINENO and will then break. Currently, this is accepted but has almost no effect, so completely ignoring assignments is probably quite safe. This also seems to happen in bash.

In ksh variants, assigning to LINENO changes an offset for future LINENO expansions (for example, if line 10 contains LINENO=1010, future LINENO references will result in a line number 1000 more than normal; a bit like #line in C). I suggest not implementing this because it will not work well with the current design for line numbers within functions.

In both bash and ksh, unsetting LINENO makes it a normal variable, and the only way to expand line numbers again is to restart the shell. Implementing this may or may not be worth it.