jedec_dimm: Use correct string length when populating sc->slotid_str
ClosedPublic
Actions

Authored by rpokala on Mar 22 2018, 1:12 AM.

Details

Reviewers

imp
cem

Commits

rS331345: jedec_dimm: Use correct string length when populating sc->slotid_str

Summary

Don't limit the copy to the size of the target string *pointer* (always
4 on 32-bit / 8 on 64-bit); limit the copy to the size of the target
string *buffer*.

THis is Coverity issue 1386912.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 15711
Build 15737: arc lint + arc unit

Event Timeline

rpokala created this revision.Mar 22 2018, 1:12 AM

Herald added a subscriber: imp. · View Herald TranscriptMar 22 2018, 1:12 AM

Harbormaster completed remote builds in B15702: Diff 40576.Mar 22 2018, 1:12 AM

imp added inline comments.Mar 22 2018, 1:15 AM

sys/dev/jedec_dimm/jedec_dimm.c
346	don't need M_ZERO here. You are going to copy a string in and initialize every byte

@imp correctly observed that since the buffer is allocated to fit, then
fully initialized, there's no need to use M_ZERO.

Harbormaster completed remote builds in B15704: Diff 40578.Mar 22 2018, 1:19 AM

rpokala marked an inline comment as done.Mar 22 2018, 1:20 AM

cem added a subscriber: cem.Mar 22 2018, 2:16 AM

cem added inline comments.

sys/dev/jedec_dimm/jedec_dimm.c
347	You could just use strdup(9) instead.

rpokala added inline comments.Mar 22 2018, 2:52 AM

sys/dev/jedec_dimm/jedec_dimm.c
347	Had I known that it existed... :-S (There is no `strdup.9` file, or any `str*.9` for that matter; as with much of libkern, we need documentation.) Starting builds with that now. I also just realized why my earlier testing never hit the problem in the first place, so I'm going to do more extensive testing when I get home.

As @cem pointed out, it's even easier to use strdup().

Harbormaster completed remote builds in B15711: Diff 40587.Mar 22 2018, 5:51 AM

what, no strdup.9 man page? Geeze :)

This is has now reached perfect: there's nothing left to take away.

This revision is now accepted and ready to land.Mar 22 2018, 5:54 AM

# Much longer than sizeof(sc->slotid_str)
% kenv hint.jedec_dimm.0.slotid
DIMM0LONGERSTRING

Before:

% sysctl dev.jedec_dimm.0.slotid
dev.jedec_dimm.0.slotid: DIMM0LO

After:

% sysctl dev.jedec_dimm.0.slotid
dev.jedec_dimm.0.slotid: DIMM0LONGERSTRING

👌

Feel free to spend a bunch of time documenting libkern if you want. It's definitely valuable and shouldn't be too hard. Mostly just tedious. Maybe a thin document that just defines all of the libc-alike aliases and gets linked in a million places would be enough to start.

In D14790#311049, @cem wrote:

Feel free to spend a bunch of time documenting libkern if you want. It's definitely valuable and shouldn't be too hard. Mostly just tedious. Maybe a thin document that just defines all of the libc-alike aliases and gets linked in a million places would be enough to start.

Yeah. Different project for a different day. :-)

rpokala closed this revision.Jun 8 2018, 3:27 AM

rpokala added a commit: rS331345: jedec_dimm: Use correct string length when populating sc->slotid_str.