INTRNAME_LEN and INTR_ISRC_NAMELEN serve extremely similar purposes.
Setting the length of interrupt name strings. Having two values with
near-identical purposes, but different values serves to confuse. In
turn this is a recipe for trouble if the wrong one is used. As such
merge them together.
Details
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 49255 Build 46145: arc lint + arc unit
Event Timeline
I had originally written D38116 before doing this. Then on review I realized this was a distinct value and that would cause things to explode. This does mean intermediate strings are shorter, but these rarely occur. As such I tend to opt for this approach and getting rid of the booby trap.
This also effects D35898 as I believe the combined length value should be used for ii_name as well.
I checked find sys -type f -print0 | xargs -0 grep -eINTR_ISRC_NAMELEN -l and no results. Could be a few spots use sizeof(isrc->isrc_name).
Looking at isrc_update_name(), we concatenate two strings, one from a buffer of size MAXCOMLEN+1, so changing INTRNAME_LEN from 2*MAXCOMLEN+1 to MAXCOMLEN+1 seems likely to introduce some string truncation? Shouldn't we keep the original, larger size?
This is true, but this is also the nature of fixed-length strings. Copy from too long a string and it will overflow. If the truncation becomes a problem, you can increase the string length. I worry more having two fixed-length strings of different lengths is a recipe for a security hole.
I dislike MAXCOMLEN not including the terminator. Also looks like rather a long time since MAXCOMLEN was last increased.
sys/kern/subr_intr.c | ||
---|---|---|
477 | FYI, I would maybe change this to use vsnprintf(isrc->isrc_name, sizeof(isrc->isrc_name), ...) I'm not sure if that approach means you can use the constant in fewer places? |
As far as it goes, I could see moving INTRNAME_LEN from sys/sys/intr.h to sys/sys/interrupt.h, then replacing all the instances of MAXCOMLEN + 1 in the PowerPC/x86 intr_machdep.c files with INTRNAME_LEN. Then INTRNAME_LEN could instead use 2*MAXCOMLEN + 1.
Ultimately though the interrupt name strings will get truncated somewhere at some limit. I see little gain by having an extra constant which merely moves where the truncation occurs and increases confusion.
sys/kern/subr_intr.c | ||
---|---|---|
477 | I agree with this, it is in my tree. Doesn't really make too large of an impact right now though. |
Updating as per @jhb's comment. Checking I noticed INTR_IPI_NAMELEN should also be part of this and merged those in.
Ping on D38115. I've got a patch/commit for moving INTRNAME_LEN to sys/sys/interrupt.h and switching PowerPC/x86 to use INTRNAME_LEN instead of MAXCOMLEN + 1. I'm wondering whether that would be acceptable for reviewing as part of D38115 (the overlap is so heavy it seems to make sense, but others may disagree).