Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

unalias should exit with non-zero status while removing undefined alias #909

Closed
siteshwar opened this issue Sep 22, 2018 · 2 comments
Closed

unalias should exit with non-zero status while removing undefined alias #909

siteshwar opened this issue Sep 22, 2018 · 2 comments
Assignees
@siteshwar
Labels
bug

Comments

@siteshwar
Copy link
Contributor

Description of problem:
uanalias should exit with non-zero status while removing undefined alias

Ksh version:
2017.0.0-devel-1837-gb29f7964-dirty

How reproducible:
Always

Steps to reproduce:

alias foo=bar
unalias foo
unalias foo

Actual results:
Last command exits with 0 status.

Expected results:
Last command should exit with non-zero status.

Additional info:
This issue was identified through yash tests (issue #859).
@siteshwar siteshwar self-assigned this Sep 22, 2018
@siteshwar siteshwar added the bug label Sep 22, 2018
@siteshwar siteshwar changed the title unalias should exit with error while removing undefined alias unalias should exit with non-zero status while removing undefined alias Sep 22, 2018
@siteshwar
Copy link
Contributor Author

yash tests require non-empty stderr if unalias fails According to this page

The standard error shall be used only for diagnostic messages.

@krader1961 Does it look like it's a hard requirement ? It should be trivial to add an error message, but if it's not a hard requirement then we need to fix the test.

@krader1961
Copy link
Contributor

It looks like a hard requirement to me according to the POSIX spec. From the "EXIT STATUS" section:

> 0
  One of the alias-name operands specified did not represent a valid alias definition, or an error occurred.

I can appreciate the argument for treating this as a "soft" error (that is, successful) since removing an alias that doesn't exist is normally harmless. But it seems to me there should be feedback that the alias did not exist. Both via the exit status and a stderr message. Primarily because the user may not have realized they mistyped the alias name.

siteshwar added a commit that referenced this issue Sep 25, 2018
@siteshwar
Add test case for removing undefined alias
2bb160b 
Related: #909
McDutchie pushed a commit to ksh93/ksh that referenced this issue Jun 11, 2020
@JohnoKing @McDutchie
The unalias builtin should return an error for non-existent aliases
f7251bc 
This commit fixes a bug that caused unalias to return a zero status
when it tries to remove an alias twice. The following set of commands
will no longer end with an error:

$ alias foo=bar
$ unalias foo
$ unalias foo && echo 'Error'

This commit is based on the fix present in ksh2020, but it has been
extended with another bugfix. The initial fix for this problem tried to
remove aliases from the alias tree without accounting for NV_NOFREE. This
caused any attempt to remove a predefined aliases (e.g. `unalias float`)
to trigger an error with free, as all predefined aliases are in read-only
memory. The fix for this problem is to set NV_NOFREE when removing aliases
from the alias tree, but only if the alias is in read-only memory. All
other aliases must be freed from memory to prevent memory leaks.

I'll also note that I am using an `isalias` variable rather than the `type`
enum from ksh2020, as the `VARIABLE` value is never used and was replaced
with a bool called `aliases` in the ksh2020 release. The `isalias` variable
is an int as the ksh93u+ codebase does not use C99 bools.

Previous discussion: att#909

- src/cmd/ksh93/bltins/typeset.c:
  Remove aliases from the alias tree by using nv_delete. NV_NOFREE
  is only used when it is necessary.

- src/cmd/ksh93/tests/alias.sh:
  Add two regression tests for the bugs fixed by this commit.

(cherry picked from commit 16d5ea9b52ba51f9d1bca115ce8f4f18e97abbc4)
McDutchie pushed a commit to ksh93/ksh that referenced this issue Jun 11, 2020
@JohnoKing @McDutchie
The unalias builtin should return an error for non-existent aliases
5d50f82 
This commit fixes a bug that caused unalias to return a zero status
when it tries to remove an alias twice. The following set of commands
will no longer end with an error:

$ alias foo=bar
$ unalias foo
$ unalias foo && echo 'Error'

This commit is based on the fix present in ksh2020, but it has been
extended with another bugfix. The initial fix for this problem tried to
remove aliases from the alias tree without accounting for NV_NOFREE. This
caused any attempt to remove a predefined aliases (e.g. `unalias float`)
to trigger an error with free, as all predefined aliases are in read-only
memory. The fix for this problem is to set NV_NOFREE when removing aliases
from the alias tree, but only if the alias is in read-only memory. All
other aliases must be freed from memory to prevent memory leaks.

I'll also note that I am using an `isalias` variable rather than the `type`
enum from ksh2020, as the `VARIABLE` value is never used and was replaced
with a bool called `aliases` in the ksh2020 release. The `isalias` variable
is an int as the ksh93u+ codebase does not use C99 bools.

Previous discussion: att#909

- src/cmd/ksh93/bltins/typeset.c:
  Remove aliases from the alias tree by using nv_delete. NV_NOFREE
  is only used when it is necessary.

- src/cmd/ksh93/tests/alias.sh:
  Add two regression tests for the bugs fixed by this commit.

(cherry picked from commit 16d5ea9b52ba51f9d1bca115ce8f4f18e97abbc4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@siteshwar siteshwar

Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@siteshwar @krader1961