#3859 closed defect (fixed)

No output from joel scripts in telnet

Reported by: Chris Johns Owned by: Chris Johns
Priority: normal Milestone: 5.1
Component: shell Version: 5
Severity: normal Keywords:
Cc: Blocked By:
Blocking:

Description

Running a joel script in a telnet session results in the output being sent to the global stdout. For example:

$ telnet 1.2.3.4
Trying 1.2.3.4...
Connected to 1.2.3.4.
Escape character is '^]'.

RTEMS Shell on /dev/pty0. Use 'help' to list commands.
[/] # cat j
#! joel
ls -las /
[/] # ./j
[/] #

The bug is a new shell main loop task will default to the global stdout, stdin etc and has no information about the parent's std handles. A joel script runs in it's own work task and does not know the telnet's std handles.

There are a related set of issues in the handling of the shell_env variable, POSIX key handling and the use of the external call rtems_shell_main_loop.

The telnet example in libbsd has:

static void
telnet_shell(char *name, void *arg)
{
	rtems_shell_env_t env;

	memset(&env, 0, sizeof(env));

	env.devname = name;
	env.taskname = "TLNT";
	env.login_check = NULL;
	env.forever = false;

	rtems_shell_main_loop(&env);
}

This is problematic as control of the env has been lost and this make backwards comptatable changes difficult. Control of this struct needs to be brought back under the shell code.

Currently the posix key is set in the parent task only when the run entry point is used. The run's created shell_env is then passed to the shell's main loop task as an argument from which it is cloned. This means an env is malloced in each run call and again in the main loop of the shell.

The current code leaks memory as repeated calls to a joel script in a shell will set the key over and over. The destructor is only called when the task is deleted. We have to assume the cleanup of any shell_env allocated externally to the shell code has to be handled externally.

Setting the key in the main loop task is problematic because telnet code such as the example in libbsd uses a local stack shell_env and the key has a destructor that blindly free's the key's memory when a task is released.

Changes:

  1. Add parent_stdout, parent_stdin, and parent_stderr to the shell_env and set to the parent's std handles.
  2. Add a managed flag to shell_env and only set when allocated by rtems_shell_init_env. Change rtems_shell_env_free to only free the shell_env if managed.
  3. Remove all key sets and have only one in the shell's main loop code.
  4. Change rtems_shell_init_env to get the current tasks key and clone that before cloning the global env.
  5. Update rtems_shell_dup_current_env to set the parent std handles.
  6. Have the main loop use the parent std handles rather than the global handles.
  7. Check the magic field has been set in the shell's main loop and raise an error if not set. The only code to set this field should reside in shell.c. Code such as libbsd will need to call rtems_shell_dup_current_env.

Change History (15)

comment:1 Changed on Feb 4, 2020 at 4:25:01 PM by Sebastian Huber

Sounds that it is currently pretty broken.

What about changing the managed member into a destroy handler which is initialized by the constructor?

comment:2 in reply to:  1 ; Changed on Feb 5, 2020 at 2:14:46 AM by Chris Johns

Replying to Sebastian Huber:

Sounds that it is currently pretty broken.

There seems to be a few overlapping issues that make things a little confusing.

What about changing the managed member into a destroy handler which is initialized by the constructor?

Yes, my patch I am testing does this. I have an issue on 4.11 where a printf line in changes what happens and I am not sure why. I need to resolve this. I will then create a 4.11 clone of this ticket and post a patch for that branch.

comment:3 in reply to:  2 Changed on Feb 10, 2020 at 9:59:43 PM by Chris Johns

Replying to Chris Johns:

I have an issue on 4.11 where a printf line in changes what happens and I am not sure why. I need to resolve this. I will then create a 4.11 clone of this ticket and post a patch for that branch.

I have chased the issue down to how we set up the re-entrant struct newlib uses in our threads. My changes follow what exists ...

  FILE *input = fopen(shell_env->input, "r");
  if (input == NULL)
     error(...);
  stdin = input;

The only calls on stdin before the assignment are fileno(stdin) and this does not touch the re-entrant struct so the first call to libc after the assignment is setvbuf(stdout, NULL, _IONBF, 0); and newlib performs it's lazy initialisation and resets the re-entrant struct.

This issue is present on 4.11 and 5 as far as I can see.

comment:4 Changed on Feb 11, 2020 at 12:26:20 AM by Chris Johns

Blocked By: 3870 added

comment:5 Changed on Feb 17, 2020 at 4:51:50 AM by Chris Johns

It is not easy making a joel test because of the way we have used the linker mapped printf to printk.

comment:6 Changed on Feb 17, 2020 at 6:42:57 AM by Sebastian Huber

Only printf(), puts(), and putchar() are wrapped. The shell related code should use fprintf() etc. Using printf() would be a bug.

comment:7 Changed on Feb 17, 2020 at 8:51:46 PM by Chris Johns

A test is not easy as a shell test cannot use any standard commands, eg echo because these call printf. The post test patch provides a command that uses fprintf and this works enough to test the shell start, main loop, and input and output support.

comment:8 Changed on Feb 18, 2020 at 2:55:20 AM by Chris Johns

Blocked By: 3870 removed

comment:9 Changed on Feb 18, 2020 at 6:06:14 AM by Sebastian Huber

It is a bug that the echo command uses putchar(). It should use fputc(c, stdout).

comment:10 in reply to:  9 Changed on Feb 18, 2020 at 6:55:17 AM by Chris Johns

Replying to Sebastian Huber:

It is a bug that the echo command uses putchar(). It should use fputc(c, stdout).

Could you please explain why?

Other commands like ls use printf.

comment:11 Changed on Feb 18, 2020 at 7:02:47 AM by Sebastian Huber

Every shell related code which uses printf(), puts(), putchar(), etc. which work on file descriptors is broken. The file descriptors are global. Only the stdin, stdout, and stderr file pointers are thread-specific.

comment:12 in reply to:  11 Changed on Feb 18, 2020 at 7:09:20 AM by Chris Johns

But ls works nicely with telnet, actually all commands I know of work well with the redirection we implement. I am sorry you have lost here with your bug comment.

My comments relate to our testsuite and the hack we have to switch printf via the linker to keep the tests as small as possible.

comment:13 Changed on Feb 18, 2020 at 7:27:06 AM by Sebastian Huber

Sorry for the confusion. I checked the Newlib code and it actually uses stdout for printf(). I thought it uses directly a file descriptor, but this is not the case. I vaguely remember that Ralf changed some printf() to fprintf(stdout, ...) for whatever reason.

comment:14 Changed on Apr 9, 2020 at 12:21:41 AM by Chris Johns

I have found the default command handler in libbsd needs updating ...

https://git.rtems.org/rtems-libbsd/tree/rtemsbsd/telnetd/telnetd-service.c#n72

The input and output fields needs have strings. I will change the shell's handling of NULL to reference the parent descriptors to catch user's code.

comment:15 Changed on Apr 14, 2020 at 10:30:49 PM by Chris Johns <chrisj@…>

Resolution: fixed
Status: assignedclosed

In d007cc2/rtems:

libmisc/shell: Fix the handling of joel scripts in telnet

  • Fix the passing of std[in/out] to child threads
  • Fix deleting of managed memory in the key destructor
  • Only set the key in the main loop thread
  • Only allocate a shell env outside of the main loop
  • Fix memory leak if the task start fails
  • Remove error level from shell env, it cannot be returned this way. Add exit_code but the API is broken so it cannot be returned.

Closes #3859

Note: See TracTickets for help on using tickets.