Opened 6 years ago

Closed 6 years ago

#9687 closed bug (fixed)

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

Reported by: diver Owned by: anevilyak
Priority: normal Milestone: R1
Component: Applications/Command Line Tools Version: R1/Development
Keywords: Cc: bonefish
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

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

Attachments (1)

kill-660-debug-20-04-2013-07-41-24.report (5.9 KB) - added by diver 6 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 6 years ago by 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.

comment:2 Changed 6 years ago by anevilyak

Owner: changed from phoudoin to anevilyak
Status: newin-progress

comment:3 Changed 6 years ago by anevilyak

Resolution: fixed
Status: in-progressclosed

Fixed in hrev45526.

comment:4 Changed 6 years ago by diver

Resolution: fixed
Status: closedreopened

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.

Changed 6 years ago by diver

comment:5 in reply to:  4 Changed 6 years ago by 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.

comment:6 Changed 6 years ago by diver

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

Last edited 6 years ago by diver (previous) (diff)

comment:7 Changed 6 years ago by 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?

comment:8 Changed 6 years ago by 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

comment:9 Changed 6 years ago by diver

Ok, kill only crashes in gcc2 builds.

comment:10 Changed 6 years ago by anevilyak

Cc: bonefish added

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?

Last edited 6 years ago by anevilyak (previous) (diff)

comment:11 Changed 6 years ago by 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.

comment:12 in reply to:  11 Changed 6 years ago by 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.

comment:13 Changed 6 years ago by anevilyak

Resolution: fixed
Status: reopenedclosed

Should be fixed in hrev45699.

Note: See TracTickets for help on using tickets.