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.

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

Issue with ksh93 on OpenBSD #776

Closed
daviduhden opened this issue Aug 3, 2024 · 16 comments
Closed

Issue with ksh93 on OpenBSD #776

daviduhden opened this issue Aug 3, 2024 · 16 comments
Labels
TODO Things to be done before releasing

Comments

@daviduhden
Copy link

daviduhden commented Aug 3, 2024

I encountered a serious problem after changing the root user's shell to ksh93 1.0.8 on OpenBSD 7.5 using the chpass(1) command.

When executing basic commands such as cd or exit, the following error messages are displayed:

For cd:

-ksh93: wcd[93]: _ignore_: line 89: local: not found

For exit:

-ksh93: wsu[97]: _ignore_: line 89: local: not found

Additionally, when executing the pwd command, the following error is displayed:

-ksh93: pwd: cannot determine present working directory [No such file or directory]

While the actions for cd and exit are still executed despite the error, the pwd command becomes completely unusable.

This issue appears to stem from the OpenBSD global initialization script for KornShell located in /etc/ksh.kshrc. The script contains a command that is not supported by ksh93. Specifically, the command local. In order to fix this problem you have to replace it by typeset.

@posguy99
Copy link

posguy99 commented Aug 3, 2024

oBSD's ksh is pdksh, not ksh88 or ksh93 (which each have different ideas about scopes).

A good discussion by Stéphane Chazelas is here.

@McDutchie
Copy link

This issue appears to stem from the OpenBSD global initialization script for KornShell located in /etc/ksh.kshrc. The script contains a command that is not supported by ksh93. Specifically, the command local. In order to fix this problem you have to replace it by typeset.

Yes, that's right. Adding local is definitely on our to-do list, but since it is expected to have dynamic scoping which ksh93 doesn't support, this is a puzzle we haven't solved yet (it does static scoping only, see Q28 here).

In the meantime, you can solve the problem by editing src/cmd/ksh93/SHOPT.sh and changing SHOPT SYSRC= to SHOPT SYSRC=0. That will stop it from detecting /etc/ksh.kshrc at compile time and disable the behaviour of loading it at startup.

@McDutchie
Copy link

Of course that's only a partial solution. The user's ~/.kshrc may be incompatible as well.

Maybe a better solution would be: on systems where the default ksh on $PATH is not ksh93 at build time, compile ksh to use variant names like /etc/ksh93.kshrc and ~/.ksh93rc by default. What do you think?

@posguy99
Copy link

posguy99 commented Aug 4, 2024

I don't agree. You are special-casing real ksh behavior in favor of things that call themselves ksh but aren't. The place for that behavior is in the shell script itself, not in the shell. Not that you'd ever get oBSD to go along with that, since pdksh is in their base.

What's next, /etc/profile and ~/.profile? That way just leads to the mess that is bash.

@McDutchie
Copy link

things that call themselves ksh but aren't

That ship sailed more than three decades ago. If AT&T hadn't insisted on such an outdated and destructive proprietary mindset from the get go, neither pdksh nor bash would ever have existed, and the community would have stepped in to fix the mess that AT&T was making of ksh.

It's AT&T that screwed up by making ksh93 incompatible with ksh88, so it's not for us now to take the arrogant position that pdksh isn't a “real” ksh. It's a 99.9% accurate clone of the ksh88 of the days (even emulating some of its bugs), which is very impressive given that the source code was a closely guarded corporate secret then.

The place for that behavior is in the shell script itself, not in the shell. Not that you'd ever get oBSD to go along with that, since pdksh is in their base.

Exactly. Rigid adherence to principles is often a good thing, but I'm not convinced of that in this particular case. This is not the first time this issue annoys people. I've run into it myself as well. Plus, I'm the one who is going to have to deal with the reports that are going to keep coming in if something isn't done about this.

It's not as if there isn't a precedent, anyway. It's common for the shell binary to be called ksh93 on systems where ksh might be pdksh or mksh. Why should the rc scripts not follow suit?

Which reminds me of the fact that ksh is already detecting what it's called at runtime to decide whether or not to default to POSIX mode (when invoked as sh). So should be easy to add a little code for it to adapt the rc script names at runtime. If it's called ksh93, it should look for matching ksh93rc script names, otherwise nothing changes.

What's next, /etc/profile and ~/.profile?

profile scripts are specified by POSIX and are expected to work on all the POSIX shells by limiting themselves to sh(1) commands. I can promise you that nothing is going to change there.

That way just leads to the mess that is bash.

Compared to the incredible mess that the AT&T folks left for us to tidy up, bash (for all its flaws) is a paragon of correctness and reliability. Chet Ramey and the FSF simply did a better job than AT&T did.

Even after fixing well over 1000 AT&T bugs, as long as we have not managed to fix bugs/design flaws like #771, we have no business calling any other shell a mess, full stop.

By the way, I do appreciate you contributing your opinion, even if I don't end up agreeing with it. In fact, it was useful as it caused me to think this whole thing through more deeply.

@posguy99
Copy link

posguy99 commented Aug 4, 2024

I don't think there's a problem with ksh adapting to the name of the binary at run-time, but I do feel 100% there's a problem with the behavior being a compile-time decision. Your comment in #776 (comment) sounded like the latter.

(BTW I will never stop thinking that the collection of how rc files work in bash is completely broken.)

@McDutchie
Copy link

I don't think there's a problem with ksh adapting to the name of the binary at run-time, but I do feel 100% there's a problem with the behavior being a compile-time decision. Your comment in #776 (comment) sounded like the latter.

Yeah, that was my original idea. I agree with you that a runtime decision is better.

(BTW I will never stop thinking that the collection of how rc files work in bash is completely broken.)

I never claimed bash is flawless :)

@McDutchie McDutchie added the TODO Things to be done before releasing label Aug 4, 2024
@daviduhden
Copy link
Author

daviduhden commented Aug 6, 2024

On systems where the default ksh on $PATH is not ksh93 at build time, compile ksh to use variant names like /etc/ksh93.kshrc and ~/.ksh93rc by default.

What would need to be modified to implement this? I would like to propose a patch in the OpenBSD port to fix this problem. It might be better if you could solve this problem in the original project by adding conditionals for operating systems that use another KornShell implementation by default, such as OpenBSD, NetBSD and MirBSD.

@McDutchie
Copy link

@daviduhden, here is a patch for you to test. Please let me know how it works for you.

As discussed above with @posguy99, it makes the decision at runtime, based on whether ksh is invoked as ksh93. If so, it looks for /etc/ksh93.kshrc and ~/.ksh93rc instead of the names without the 93. Since our ksh is generally installed as ksh93 on systems that have another ksh as the default, this should meet your needs.

Patch v1
diff --git a/src/cmd/ksh93/Mamfile b/src/cmd/ksh93/Mamfile
index 5a21008c..8cc3cc44 100644
--- a/src/cmd/ksh93/Mamfile
+++ b/src/cmd/ksh93/Mamfile
@@ -119,8 +119,8 @@ make install virtual
 		exec -				writedef PRINTF_LEGACY 1 ;;
 		exec -			esac ;;
 		exec -		'SYSRC=')
-		exec -			# if one of these exists, make SHOPT_SYSRC load /etc/ksh.kshrc by default
-		exec -			if	test -f /etc/ksh.kshrc || test -f /etc/bash.bashrc
+		exec -			# if one of these exists, make SHOPT_SYSRC load /etc/ksh{,93}.kshrc by default
+		exec -			if	test -f /etc/ksh.kshrc || test -f /etc/ksh93.kshrc || test -f /etc/bash.bashrc
 		exec -			then	writedef SYSRC 1
 		exec -			fi ;;
 		exec -		# some other SHOPTs may be probed for using feature tests in features/options
diff --git a/src/cmd/ksh93/data/msg.c b/src/cmd/ksh93/data/msg.c
index 698daeb0..7407fe8b 100644
--- a/src/cmd/ksh93/data/msg.c
+++ b/src/cmd/ksh93/data/msg.c
@@ -181,6 +181,7 @@ const char e_suidprofile[]	= "/etc/suid_profile";
 #endif
 #if SHOPT_SYSRC
 const char e_sysrc[]		= "/etc/ksh.kshrc";
+const char e_sysrc93[]		= "/etc/ksh93.kshrc";
 #endif
 #if !SHOPT_SCRIPTONLY
 const char hist_fname[]		= "/.sh_history";
diff --git a/src/cmd/ksh93/include/defs.h b/src/cmd/ksh93/include/defs.h
index 568d6aee..39b0925d 100644
--- a/src/cmd/ksh93/include/defs.h
+++ b/src/cmd/ksh93/include/defs.h
@@ -92,6 +92,7 @@ extern char*	sh_setenviron(const char*);
 #define SH_TYPE_KSH		002
 #define SH_TYPE_POSIX		004
 #define SH_TYPE_LOGIN		010
+#define SH_TYPE_KSH93		020
 #define SH_TYPE_RESTRICTED	040
 
 #ifndef PIPE_BUF
diff --git a/src/cmd/ksh93/include/io.h b/src/cmd/ksh93/include/io.h
index f3c8ca78..7a0dc91a 100644
--- a/src/cmd/ksh93/include/io.h
+++ b/src/cmd/ksh93/include/io.h
@@ -110,6 +110,7 @@ extern const char	e_profile[];
 extern const char	e_sysprofile[];
 #if SHOPT_SYSRC
 extern const char	e_sysrc[];
+extern const char	e_sysrc93[];
 #endif
 extern const char	e_stdprompt[];
 extern const char	e_supprompt[];
diff --git a/src/cmd/ksh93/include/shell.h b/src/cmd/ksh93/include/shell.h
index 0792a047..694b1cb7 100644
--- a/src/cmd/ksh93/include/shell.h
+++ b/src/cmd/ksh93/include/shell.h
@@ -403,6 +403,9 @@ struct Shell_s
 #if SHOPT_REGRESS
 	struct Regress_s *regress;
 #endif /* SHOPT_REGRESS */
+#if SHOPT_SYSRC
+	char		*sysrc;		/* path to system-wide kshrc */
+#endif
 };
 
 /* used for builtins */
diff --git a/src/cmd/ksh93/sh/init.c b/src/cmd/ksh93/sh/init.c
index 6dce5b41..03eed38a 100644
--- a/src/cmd/ksh93/sh/init.c
+++ b/src/cmd/ksh93/sh/init.c
@@ -1258,7 +1258,10 @@ int sh_type(const char *path)
 		s++;
 		t |= SH_TYPE_SH;
 		if ((t & SH_TYPE_KSH) && *s == '9' && *(s+1) == '3')
+		{
 			s += 2;
+			t |= SH_TYPE_KSH93;
+		}
 #if _WINIX
 		if (*s == '.' && *(s+1) == 'e' && *(s+2) == 'x' && *(s+3) == 'e')
 			s += 4;
@@ -1375,9 +1378,12 @@ Shell_t *sh_init(int argc,char *argv[], Shinit_f userinit)
 	}
 	/* read the environment */
 	env_init();
+#if SHOPT_SYSRC
+	sh.sysrc = type & SH_TYPE_KSH93 ? e_sysrc93 : e_sysrc;
+#endif
 	if(!ENVNOD->nvalue.cp)
 	{
-		sfprintf(sh.strbuf,"%s/.kshrc",nv_getval(HOME));
+		sfprintf(sh.strbuf, type & SH_TYPE_KSH93 ? "%s/.ksh93rc" : "%s/.kshrc", nv_getval(HOME));
 		nv_putval(ENVNOD,sfstruse(sh.strbuf),NV_RDONLY);
 	}
 	/* increase SHLVL */
diff --git a/src/cmd/ksh93/sh/main.c b/src/cmd/ksh93/sh/main.c
index 4bd97845..820db4cf 100644
--- a/src/cmd/ksh93/sh/main.c
+++ b/src/cmd/ksh93/sh/main.c
@@ -209,7 +209,7 @@ int sh_main(int ac, char *av[], Shinit_f userinit)
 					name = *name ? sh_strdup(name) : NULL;
 #if SHOPT_SYSRC
 				if(!strmatch(name, "?(.)/./*"))
-					sh_source(iop, e_sysrc);
+					sh_source(iop, sh.sysrc);
 #endif
 				if(name)
 				{

@daviduhden
Copy link
Author

Here is a patch for you to test. Please let me know how it works for you.

I have tested the patch on OpenBSD 7.6-beta, and ksh93u+m now functions perfectly. I had to make some adjustments to the diff for init.c as it did not apply to version 1.0.10. The patches have already been proposed for the OpenBSD port along with an update to version 1.0.10. Thank you for your attention. It is rare to see a developer work so diligently to resolve issues that arise with the software they maintain.

@pstumpf
Copy link

pstumpf commented Aug 11, 2024

OpenBSD port maintainer here.

I do agree that we should probably patch out sourcing of /etc/ksh.kshrc (if need be, even just locally in the OpenBSD port), but changing the default user initialisation file to $HOME/.ksh93rc is way over the top and will break existing user setups not only on OpenBSD, but most Linux distributions. One should generally be able to maintain a .kshrc that works with different versions of ksh.

So I think the simplest solution would be to just make the value of e_sysrc[] configurable at build time.

@McDutchie
Copy link

Thanks for the feedback. I will give this some more thought.

@pstumpf
Copy link

pstumpf commented Aug 21, 2024

Oh, and … does the Mamfile really check for existence of /etc/ksh.kshrc or /etc/bash.bashrc on the build machine and base its decision of whether to include the sysrc feature on the existence of one of those files? That's just wrong. I think SHOPT_SYSRC should always be included. It’s also documented this way.

@McDutchie
Copy link

Oh, and … does the Mamfile really check for existence of /etc/ksh.kshrc or /etc/bash.bashrc on the build machine and base its decision of whether to include the sysrc feature on the existence of one of those files? That's just wrong. I think SHOPT_SYSRC should always be included.

ksh93 has always worked this way by default, even in the AT&T days. You can edit src/cmd/ksh93/SHOPT.sh and set SYSRC to 1 to always enable /etc/ksh.kshrc or to 0 to always disable it. The way to do that for a port would be through a local patch.

It’s also documented this way.

It isn't. src/cmd/ksh93/README quite clearly documents the current practice. In SHOPT.sh itself, it's mentioned that an empty value means probe.

@pstumpf
Copy link

pstumpf commented Aug 26, 2024

Then it is broken since AT&T days. A README in the source code is not user-facing documentation, and the man page does not give any clear indication if ksh supports the sysrc feature on a given system.

The check for /etc/ksh.kshrc may have made some semblance of sense "back in the day" -- nowadays it just leads to confusion and unexpected behaviour for both package maintainers and users. You’ll even end up with a different Korn Shell depending on if bash is installed or not on the build machine.

This is exactly one of those behaviours this project should fix by providing some sensible default.

@McDutchie
Copy link

I concede the point. The SHOPT_SYSRC probe is broken.

As for the rest of this issue, it seems like someone is going to be unhappy no matter what I decide, so I'm deciding to change as little as necessary to achieve sanity.

The SHOPT_SYSRC probe is deleted from the Mamfile and SHOPT_SYSRC is now enabled by default. On systems such as NetBSD and OpenBSD, where /etc/ksh.kshrc is incompatible by default, users building ksh93 will have to either change its value to 0 to disable loading /etc/ksh.kshrc at startup, or edit /etc/ksh.kshrc to make it compatible with ksh93.

I will add a paragraph to README.md to make users (and packagers) aware of this.

McDutchie added a commit that referenced this issue Sep 3, 2024
It's just not right that it depends on the presence of
/etc/ksh.kshrc or even /etc/bash.bashrec on the build machine
whether /etc/ksh.kshrc is going to be loaded at startup on the
users' machine.

src/cmd/ksh93/Mamfile:
- Remove SHOPT_SYSRC probe code.

src/cmd/ksh93/SHOPT.sh:
- Enable SHOPT_SYSRC by default.

README.md:
- Document that SHOPT_SYSRC should be disabled on systems with a
  default incompatible /etc/ksh.kshrc.

Resolves: #776
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO Things to be done before releasing
Projects
None yet
Development

No branches or pull requests

4 participants