[bin:kill] crashes if pid doesn't exit

[diver]

If one tries to kill by pid, if e.g. the pid doesn't exit, kill crashes and one sees the message twice:

~/> kill 1234 kill: 1234: No such process kill: 1234: No such process

Also kill doesn't print the message at all if one tries to kill by process name which doesn't exist:

~/> kill foo

[anevilyak]

Interestingly, I can't reproduce the crashing part on my machine over here, though I do see the doubled messages. One thing that strikes me of interest is that kill seems to assume/rely on the argv array being NULL terminated, which I didn't think was required/part of the standard, so I could see how it might potentially crash.

[anevilyak]

Fixed in hrev45526.

[diver]

Still crashes in hrev45531. It also prints "Invalid argument" in case a process name doesn't exist. I think "No such process (name") would be better.

[anevilyak]

Replying to diver:

It also prints "Invalid argument" in case a process name doesn't exist. I think "No such process (name") would be better.

Changed it to also return no such process in that case. As far as the crash goes however, I'm completely unable to reproduce it, and going off your report it actually happens as the team is exiting. Did this definitely only start with hrev45523 or does hrev45518 and/or hrev45522 already exhibit the crash? I really don't see anything in 23 that could be causing that behavior specifically.

[diver]

Checked with a recent nightly (hrev45526) and it exhibits the same behavior - crashes.

[anevilyak]

That's too new. I wanted you to try it with one older than hrev45523 because that's where the kill by name functionality was introduced, and I want to eliminate that patch as a possibility. I suspect this might be another ASLR victim, and that was introduced in hrev45518, so something between 18 and 23 would be ideal. Also, anything of interest in syslog when it crashes?

[diver]

git checkout hrev45518

~> type kill kill is a shell builtin kill 12345 - doesn't crash

# reverting to pre-aslr merge git checkout hrev45517 # updating to HEAD git checkout origin src/bin

~> type kill kill is /bin/kill kill 12345 - crashes

[diver]

Ok, kill only crashes in gcc2 builds.

[anevilyak]

Problem found: at ​http://cgit.haiku-os.org/haiku/tree/src/bin/coreutils/src/kill.c#L297 send_signals() calls is_number, casting its pid argument to an intmax_t. However, on Haiku, pid_t is 32-bit while intmax_t is 64-bit. Consequently, when is_number() writes the resulting value back at ​http://cgit.haiku-os.org/haiku/tree/src/bin/coreutils/src/kill.c#n235 , it also smashes the next 4 bytes on the stack. AFAICT from opengroup.org, our pid_t and intmax_t definitions are both spec compliant though, so I'm uncertain as to how this code manages to work OK on other platforms. The simple solution would seem to be to change is_number to simply accept a pid_t* instead. Thoughts?

[bonefish]

I would revert the is_number() and send_signals() part of the original patch (cc2c83fa5ce13347f19da08a40f43c256e96d9a6). send_signals() already did a (correct!) check for a number. The only thing to be changed there is calling kill_by_name() instead of failing. Also, "OS.h" is not a local header.

That being said, I'm not really happy about disabling the bash built-in kill, as that removes the feature of killing jobs. So, if you ask me, I'd revert the commit (and its fixes) until there's also a patch for the shell built-in.

[anevilyak]

Replying to bonefish:

I would revert the is_number() and send_signals() part of the original patch (cc2c83fa5ce13347f19da08a40f43c256e96d9a6). send_signals() already did a (correct!) check for a number. The only thing to be changed there is calling kill_by_name() instead of failing. Also, "OS.h" is not a local header.

Ah, I'd completely forgotten all of that was from a patch, thanks for the hint.

That being said, I'm not really happy about disabling the bash built-in kill, as that removes the feature of killing jobs. So, if you ask me, I'd revert the commit (and its fixes) until there's also a patch for the shell built-in.

Could certainly do that, will have a look and see what patching the latter would take.

[anevilyak]

Should be fixed in hrev45699.