[Toybox] Some questions re: init...

[Rob Landley]

On 01/05/14 20:31, ibid.ag at gmail.com wrote:
> On Sun, Jan 05, 2014 at 11:49:31AM -0600, Rob Landley wrote:
>> On 01/01/14 03:26, ibid.ag at gmail.com wrote:
>>> I was looking over init, and noticed some things that seemed possibly
>>> less than optimal, as well as one detail that seemed illogical.
>>
>> I haven't done any cleanup on this one yet. It could be doing anything...
>>
>>> First, the oddity:
>>>    p = getenv("TERM");
>>> #ifdef VT_OPENQRY
>>>    int terminal_no;
>>>    if (ioctl(0, VT_OPENQRY, &terminal_no)) {
>>>      if (!p || !strcmp(p,"linux")) putenv((char*)"TERM=vt102");
>>>    } else
>>> #endif
>>>    if (!p) putenv((char*)"TERM=linux");
>>>
>>> As I read it, this code will set TERM to vt102 if
>>> (1) the VT_OPENQRY ioctl is supported, AND
>>> (2) TERM is not set or is set to "linux".
>>
>> Now I'm curious: typecasting a string constant to char * in your
>> above quoted snippet is _crazy_, but it's idiosyncratic crazy of a
>> kind that's easy to track. I.E. it looks like an easter egg:
>>
>> $ grep vt102 busybox/init/init.c
>>    putenv((char*)"TERM=vt102");
>>
>> And busybox is doing it too. Presumably copied from the same BSD
>> source. Neither ever asked "is there a reason to do this?" Sigh.
>
> It's not in any BSD init.
> (I've checked for "(char*)" and "vt102" in NetBSD, MirOS, BSDI,
> BSD-lite2, and FreeBSD init.c--neither is there.)

I actually looked at the submitted init more closely (did some 
whitespace cleanup over lunch) and it's doing (char *) typecasting on a 
bunch of string constants. Not sure why it's doing that...

> OK...vt102 is supposed to be "if it's a serial line", and busybox had it
> from the beginning.

I'm not hugely worried about being sued over a 2 line code snippet, I'm 
just trying to keep the code clean. I often worry about things that 
aren't necessarily an immediate issue. :)

> (char*)"string" dates to 06c0a71d23/Jan 27, 2007, and is as preparation
> for -Wwrite-strings.
> Which is to say, the result of some gcc/GNU insanity.
> Judging by the number of times it pops up in pending/, I guess that
> Ashwini's team is used to turning that warning on.

Ok. I'm still removing the unnecessary typecast when I do cleanup.

I'm aware that feeding a string constant to putenv() means that 
replacing the environment variable later with setenv() might try to free 
a non-heap memory allocation and do Bad Things. I don't see how the 
typecast helps anything.

Note this means we're trusting exec() to clean up the different 
allocation contexts for child processes, which means this is another 
case where we can't safely do fork() and toy_exec(), meaning we should 
either use execlp() or similar, or else have some way to signal xexec() 
it can't toy_exec(). (I mentioned earlier a possible toys.exec_count so 
it didn't stack forever...)

Getting familiar with environment variable allocation stuff is one of 
those weird corner cases I researched for shell implementation way back 
when (http://landley.net/notes-2006.html#24-10-2006) but it all boiled 
down to "the libc does something, and what varies". At the time uClibc 
just leaked environment variables meory all the time. No idea what musl 
does...

>> And yes, BSD is viral. People tend to assume that BSD and GPL are
>> compatible in the absence of the advertising clause, but BSD's "copy
>> this text verbatim into anything that uses even a snippet of it"
>> could be viewed as an 'additional restriction' in GPL terms, and
>> trying to hide the conflict by copying the BSD license to the _end_
>> of the file with the GPL boilerplate up front just makes the problem
>> less obvious, it doesn't diminish it. Yes, people do that:
>>
>>    http://git.busybox.net/busybox/tree/networking/ping.c
>>
>
> Mentioned before. I suspect the rationale is "The GPL requires you to
> keep the GPL/copyright notice, so BSD requiring the BSD/copyright notice
> is fine."
> I have doubts about relicensing, though.

When I relicensed my complete rewrite of Julian Seward's BSD-licensed 
bzip2 code last decade, I emailed him to ask if it was ok first. :)

>> I'm pretty happy just hardwiring in TERM=linux if it's not already
>> set. (Although possibly this is the shell's job and not init's? Sad
>> there's no spec for init...)
>
> The shell doesn't set TERM; it reads it.
> It's the job of "whatever provides a tty", which in this case means init
> is on the hook.

I was recently looking at login, which can also set it. And getty can 
set it too. (I wonder what upstart, openrc, and android's init do?)

These days termio is all software talking to other software, we emulate 
terminals so terminal emulators can parse the data, they just have to 
agree on it. There aren't anymore vt100 tn3270 or ASR33 teletypes lying 
around anymore. It's possible that minicom is brain damaged and 
hardwired, but the microcom in busybox just dumps I/O to the terminal 
it's running in. Same for netcat in toybox. Those can handle TERM=linux 
on a serial line. And qemu's -nographic mode connecting /dev/console to 
stdin and stdout works with TERM=linux on the (emulated) serial line.

So I don't see a big reason not to hardwire TERM=linux and if somebody 
does need to set it to vt102, gettty or login can do that...

>>> -final_run will panic if fork()/vfork() fails (line 236).
>>
>> Yeah, it shouldn't. That can happen because of a forkbomb. The
>> traditional init behavior is to just wait it out.
>>
>> It's possible init should be able to do something more proactive in
>> that case (killall -1 and relaunch daemons if it persists for more
>> than 15 seconds maybe), but that's beyond the domain of sysvinit and
>> into containers...
>>
>>> -Do we need to exit on failure to write informational messages?
>>> I'm thinking of lines 55 (failure to open console) and 439 ("started
>>> init" message).
>>
>> Nope. We inherited a console from somewhere (in the form of pid
>> 0-2), if we can't open the new one keep using the old one.
>
> So those two need changing.

There's plenty of cleanup to do. I've got your patch queued in my todo 
but haven't read through it yet. (Gotta adjust it for the whitespace 
changes I made today, but that's not a big deal...)

Thanks,

Rob

 1388988471.0