Page MenuHomeFreeBSD
Differential D7116

Include command line arguments in core dump process info.
ClosedPublic
Actions

Authored by jhb on Jul 5 2016, 7:37 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 22, 2:45 AM
Unknown Object (File)
Fri, Mar 14, 11:39 AM
Unknown Object (File)
Mar 8 2025, 4:01 PM
Unknown Object (File)
Mar 3 2025, 10:24 PM
Unknown Object (File)
Mar 1 2025, 10:55 PM
Unknown Object (File)
Feb 18 2025, 11:00 AM
Unknown Object (File)
Jan 30 2025, 10:36 AM
Unknown Object (File)
Jan 10 2025, 4:58 PM
View All 80 Files
Subscribers
imp

Details

Reviewers
kib
emaste
Commits
rS302859: Include command line arguments in core dump process info.
Summary

Include command line arguments in core dump process info.

Fill in pr_psargs in the NT_PRSINFO ELF core dump note with command
line arguments.

Test Plan
  • generate core dumps with the change and open them in gdb
  • was not able to test lldb as lldb doesn't examine NT_PRSINFO on FreeBSD at all (and it uses the Linux PRSTATUS parser on FreeBSD cores which is wrong. Fixing lldb means writing ELF core note parsers for the FreeBSD core notes).

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4432
Build 4482: arc lint + arc unit

Event Timeline

jhb updated this revision to Diff 18158.Jul 5 2016, 7:37 PM
jhb retitled this revision from to Include command line arguments in core dump process info..
jhb updated this object.
jhb edited the test plan for this revision. (Show Details)
jhb added reviewers: kib, emaste.
Herald added a subscriber: imp. · View Herald TranscriptJul 5 2016, 7:37 PM
kib added inline comments.Jul 6 2016, 2:16 AM
sys/kern/imgact_elf.c
1851

I think this is too limiting. If p_args is NULL, you should use proc_getargv() to read the arguments from the process UVA directly. p_args is not guaranteed to exist.

jhb added inline comments.Jul 6 2016, 12:39 PM
sys/kern/imgact_elf.c
1851

Humm, I guess I could do that. Note that pr_psargs is only 80 chars long. One thing that is annoying about proc_get_argv() is that I'll need to change it to take a maxlength, but I can do that. (I guess I had assumed that this is less useful if you have a really long command line and that any short command lines should have a valid p_args)

jhb updated this revision to Diff 18238.Jul 9 2016, 12:06 PM
  • Use proc_getargv() if p_args is NULL.
  • Set len in the proc_getargv() case to avoid trashing random memory.
  • Fix 'len' when the sbuf overflows.
kib accepted this revision.Jul 9 2016, 3:20 PM
kib edited edge metadata.
This revision is now accepted and ready to land.Jul 9 2016, 3:20 PM
Closed by commit rS302859: Include command line arguments in core dump process info. (authored by jhb). · Explain WhyJul 14 2016, 11:20 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sys/
kern/
26 lines
usr.bin/
gcore/
16 lines

Diff 18158

View Options

sys/kern/imgact_elf.c

Show First 20 Lines • Show All 1,358 Lines • ▼ Show 20 Lines
* *
* @lldev Per-adapter data * @lldev Per-adapter data
* @ifreqp Interface request * @ifreqp Interface request
*/ */
int int
xge_ioctl_stats(xge_lldev_t *lldev, struct ifreq *ifreqp) xge_ioctl_stats(xge_lldev_t *lldev, struct ifreq *ifreqp)
{ {
xge_hal_status_e status = XGE_HAL_OK; xge_hal_status_e status = XGE_HAL_OK;
char *data = (char *)ifreqp->ifr_data; char tmpdata;
char *data;
void *info = NULL; void *info = NULL;
int retValue = EINVAL; int retValue = EINVAL;
tmpdata = fubyte(ifreqp->ifr_data);
if (tmpdata == -1)
return (EFAULT);
data = &tmpdata;
switch(*data) { switch(*data) {
jhbUnsubmitted
Not Done
Inline Actions

If this is the only use, I would actually prefer to kill 'data' entirely to avoid someone trying to use additional bytes and instead do something like:

char query;
...

query = fubyte(ifreqp->ifr_data);
if (query == -1)
     return (EFAULT);

switch (query) {

Also, I wonder if we should prefer fuebyte() in new code instead? (Maybe kib@ has an opinion on that)

jhb: If this is the only use, I would actually prefer to kill 'data' entirely to avoid someone…
brooksAuthorUnsubmitted
Not Done
Inline Actions

data is used later or I would have done that. We don't currently have fuebyte()

brooks: `data` is used later or I would have done that. We don't currently have fuebyte()
case XGE_QUERY_STATS: case XGE_QUERY_STATS:
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_stats_hw(lldev->devh, status = xge_hal_stats_hw(lldev->devh,
(xge_hal_stats_hw_info_t **)&info); (xge_hal_stats_hw_info_t **)&info);
mtx_unlock(&lldev->mtx_drv); mtx_unlock(&lldev->mtx_drv);
if(status == XGE_HAL_OK) { if(status == XGE_HAL_OK) {
if(copyout(info, ifreqp->ifr_data, if(copyout(info, ifreqp->ifr_data,
sizeof(xge_hal_stats_hw_info_t)) == 0) sizeof(xge_hal_stats_hw_info_t)) == 0)
▲ Show 20 Lines • Show All 112 Lines • ▼ Show 20 Lines if(copyout(&lldev->driver_stats, ifreqp->ifr_data,
sizeof(int)) == 0) sizeof(int)) == 0)
retValue = 0; retValue = 0;
break; break;
case XGE_SET_BUFFER_MODE_1: case XGE_SET_BUFFER_MODE_1:
case XGE_SET_BUFFER_MODE_2: case XGE_SET_BUFFER_MODE_2:
case XGE_SET_BUFFER_MODE_5: case XGE_SET_BUFFER_MODE_5:
*data = (*data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N'; *data = (*data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N';
if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0) if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0)
jhbUnsubmitted
Done
Inline Actions

So this is the only place that uses 'data' and it is wrong (sizeof is wrong). I would probably fix this by not abusing data but just using a local 'char mode' and passing '&mode' to copyout().

jhb: So this is the only place that uses 'data' and it is wrong (sizeof is wrong). I would…
retValue = 0; retValue = 0;
break; break;
default: default:
xge_trace(XGE_TRACE, "Nothing is matching"); xge_trace(XGE_TRACE, "Nothing is matching");
retValue = ENOTTY; retValue = ENOTTY;
break; break;
} }
return retValue; return retValue;
} }
/** /**
* xge_ioctl_registers * xge_ioctl_registers
* IOCTL to get registers * IOCTL to get registers
* *
* @lldev Per-adapter data * @lldev Per-adapter data
* @ifreqp Interface request * @ifreqp Interface request
*/ */
int int
xge_ioctl_registers(xge_lldev_t *lldev, struct ifreq *ifreqp) xge_ioctl_registers(xge_lldev_t *lldev, struct ifreq *ifreqp)
{ {
xge_register_t *data = (xge_register_t *)ifreqp->ifr_data; xge_register_t tmpdata;
xge_register_t *data;
xge_hal_status_e status = XGE_HAL_OK; xge_hal_status_e status = XGE_HAL_OK;
int retValue = EINVAL, offset = 0, index = 0; int retValue = EINVAL, offset = 0, index = 0;
int error;
u64 val64 = 0; u64 val64 = 0;
error = copyin(ifreqp->ifr_data, &tmpdata, sizeof(tmpdata));
if (error != 0)
return (error);
data = &tmpdata;
/* Reading a register */ /* Reading a register */
if(strcmp(data->option, "-r") == 0) { if(strcmp(data->option, "-r") == 0) {
data->value = 0x0000; data->value = 0x0000;
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_mgmt_reg_read(lldev->devh, 0, data->offset, status = xge_hal_mgmt_reg_read(lldev->devh, 0, data->offset,
&data->value); &data->value);
mtx_unlock(&lldev->mtx_drv); mtx_unlock(&lldev->mtx_drv);
if(status == XGE_HAL_OK) { if(status == XGE_HAL_OK) {
if(copyout(data, ifreqp->ifr_data, sizeof(xge_register_t)) == 0) if(copyout(data, ifreqp->ifr_data, sizeof(xge_register_t)) == 0)
jhbUnsubmitted
Not Done
Inline Actions

The fact that it does copyout() already makes this function even more amusing.

jhb: The fact that it does copyout() already makes this function even more amusing.
brooksAuthorUnsubmitted
Not Done
Inline Actions

The strcmp() cases will certainly not trigger in most cases because option is a char[2] followed by an int64_t so there's garbage in the pad unless the did a bzero first.

This driver is best fixed with svn rm, but I'm changing ifr_data use so needed to do a little fixing.

brooks: The strcmp() cases will certainly not trigger in most cases because option is a char[2]…
retValue = 0; retValue = 0;
} }
} }
/* Writing to a register */ /* Writing to a register */
else if(strcmp(data->option, "-w") == 0) { else if(strcmp(data->option, "-w") == 0) {
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_mgmt_reg_write(lldev->devh, 0, data->offset, status = xge_hal_mgmt_reg_write(lldev->devh, 0, data->offset,
data->value); data->value);
▲ Show 20 Lines • Show All 1,982 Lines • Show Last 20 Lines
View Options

usr.bin/gcore/elfcore.c

Show First 20 Lines • Show All 1,358 Lines • ▼ Show 20 Lines
* *
* @lldev Per-adapter data * @lldev Per-adapter data
* @ifreqp Interface request * @ifreqp Interface request
*/ */
int int
xge_ioctl_stats(xge_lldev_t *lldev, struct ifreq *ifreqp) xge_ioctl_stats(xge_lldev_t *lldev, struct ifreq *ifreqp)
{ {
xge_hal_status_e status = XGE_HAL_OK; xge_hal_status_e status = XGE_HAL_OK;
char *data = (char *)ifreqp->ifr_data; char tmpdata;
char *data;
void *info = NULL; void *info = NULL;
int retValue = EINVAL; int retValue = EINVAL;
tmpdata = fubyte(ifreqp->ifr_data);
if (tmpdata == -1)
return (EFAULT);
data = &tmpdata;
switch(*data) { switch(*data) {
jhbUnsubmitted
Not Done
Inline Actions

If this is the only use, I would actually prefer to kill 'data' entirely to avoid someone trying to use additional bytes and instead do something like:

char query;
...

query = fubyte(ifreqp->ifr_data);
if (query == -1)
     return (EFAULT);

switch (query) {

Also, I wonder if we should prefer fuebyte() in new code instead? (Maybe kib@ has an opinion on that)

jhb: If this is the only use, I would actually prefer to kill 'data' entirely to avoid someone…
brooksAuthorUnsubmitted
Not Done
Inline Actions

data is used later or I would have done that. We don't currently have fuebyte()

brooks: `data` is used later or I would have done that. We don't currently have fuebyte()
case XGE_QUERY_STATS: case XGE_QUERY_STATS:
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_stats_hw(lldev->devh, status = xge_hal_stats_hw(lldev->devh,
(xge_hal_stats_hw_info_t **)&info); (xge_hal_stats_hw_info_t **)&info);
mtx_unlock(&lldev->mtx_drv); mtx_unlock(&lldev->mtx_drv);
if(status == XGE_HAL_OK) { if(status == XGE_HAL_OK) {
if(copyout(info, ifreqp->ifr_data, if(copyout(info, ifreqp->ifr_data,
sizeof(xge_hal_stats_hw_info_t)) == 0) sizeof(xge_hal_stats_hw_info_t)) == 0)
▲ Show 20 Lines • Show All 112 Lines • ▼ Show 20 Lines if(copyout(&lldev->driver_stats, ifreqp->ifr_data,
sizeof(int)) == 0) sizeof(int)) == 0)
retValue = 0; retValue = 0;
break; break;
case XGE_SET_BUFFER_MODE_1: case XGE_SET_BUFFER_MODE_1:
case XGE_SET_BUFFER_MODE_2: case XGE_SET_BUFFER_MODE_2:
case XGE_SET_BUFFER_MODE_5: case XGE_SET_BUFFER_MODE_5:
*data = (*data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N'; *data = (*data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N';
if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0) if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0)
jhbUnsubmitted
Done
Inline Actions

So this is the only place that uses 'data' and it is wrong (sizeof is wrong). I would probably fix this by not abusing data but just using a local 'char mode' and passing '&mode' to copyout().

jhb: So this is the only place that uses 'data' and it is wrong (sizeof is wrong). I would…
retValue = 0; retValue = 0;
break; break;
default: default:
xge_trace(XGE_TRACE, "Nothing is matching"); xge_trace(XGE_TRACE, "Nothing is matching");
retValue = ENOTTY; retValue = ENOTTY;
break; break;
} }
return retValue; return retValue;
} }
/** /**
* xge_ioctl_registers * xge_ioctl_registers
* IOCTL to get registers * IOCTL to get registers
* *
* @lldev Per-adapter data * @lldev Per-adapter data
* @ifreqp Interface request * @ifreqp Interface request
*/ */
int int
xge_ioctl_registers(xge_lldev_t *lldev, struct ifreq *ifreqp) xge_ioctl_registers(xge_lldev_t *lldev, struct ifreq *ifreqp)
{ {
xge_register_t *data = (xge_register_t *)ifreqp->ifr_data; xge_register_t tmpdata;
xge_register_t *data;
xge_hal_status_e status = XGE_HAL_OK; xge_hal_status_e status = XGE_HAL_OK;
int retValue = EINVAL, offset = 0, index = 0; int retValue = EINVAL, offset = 0, index = 0;
int error;
u64 val64 = 0; u64 val64 = 0;
error = copyin(ifreqp->ifr_data, &tmpdata, sizeof(tmpdata));
if (error != 0)
return (error);
data = &tmpdata;
/* Reading a register */ /* Reading a register */
if(strcmp(data->option, "-r") == 0) { if(strcmp(data->option, "-r") == 0) {
data->value = 0x0000; data->value = 0x0000;
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_mgmt_reg_read(lldev->devh, 0, data->offset, status = xge_hal_mgmt_reg_read(lldev->devh, 0, data->offset,
&data->value); &data->value);
mtx_unlock(&lldev->mtx_drv); mtx_unlock(&lldev->mtx_drv);
if(status == XGE_HAL_OK) { if(status == XGE_HAL_OK) {
if(copyout(data, ifreqp->ifr_data, sizeof(xge_register_t)) == 0) if(copyout(data, ifreqp->ifr_data, sizeof(xge_register_t)) == 0)
jhbUnsubmitted
Not Done
Inline Actions

The fact that it does copyout() already makes this function even more amusing.

jhb: The fact that it does copyout() already makes this function even more amusing.
brooksAuthorUnsubmitted
Not Done
Inline Actions

The strcmp() cases will certainly not trigger in most cases because option is a char[2] followed by an int64_t so there's garbage in the pad unless the did a bzero first.

This driver is best fixed with svn rm, but I'm changing ifr_data use so needed to do a little fixing.

brooks: The strcmp() cases will certainly not trigger in most cases because option is a char[2]…
retValue = 0; retValue = 0;
} }
} }
/* Writing to a register */ /* Writing to a register */
else if(strcmp(data->option, "-w") == 0) { else if(strcmp(data->option, "-w") == 0) {
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_mgmt_reg_write(lldev->devh, 0, data->offset, status = xge_hal_mgmt_reg_write(lldev->devh, 0, data->offset,
data->value); data->value);
▲ Show 20 Lines • Show All 1,982 Lines • Show Last 20 Lines