Page MenuHomeFreeBSD

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

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

Details

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
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 15711
Build 15737: arc lint + arc unit

Event Timeline

rpokala created this revision.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

rpokala updated this revision to Diff 40578.Mar 22 2018, 1:19 AM

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

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.

rpokala updated this revision to Diff 40587.Mar 22 2018, 5:51 AM

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

imp accepted this revision.Mar 22 2018, 5:54 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
rpokala marked 2 inline comments as done.Mar 22 2018, 5:58 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
cem accepted this revision.Mar 22 2018, 6:11 AM

๐Ÿ‘Œ

cem added a comment.Mar 22 2018, 6:13 AM

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. :-)