Opened 12 years ago
Closed 12 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: | ||
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)
Change History (14)
comment:1 by , 12 years ago
comment:2 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → in-progress |
follow-up: 5 comment:4 by , 12 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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.
by , 12 years ago
Attachment: | kill-660-debug-20-04-2013-07-41-24.report added |
---|
comment:5 by , 12 years ago
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 by , 12 years ago
Checked with a recent nightly (hrev45526) and it exhibits the same behavior - crashes.
comment:7 by , 12 years ago
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 by , 12 years ago
comment:10 by , 12 years ago
Cc: | 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?
follow-up: 12 comment:11 by , 12 years ago
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 by , 12 years ago
Replying to bonefish:
I would revert the
is_number()
andsend_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 callingkill_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 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Should be fixed in hrev45699.
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.