On 10/23/18 8:15 AM, Jan Tulak wrote: > On Tue, Oct 23, 2018 at 2:10 PM Eric Sandeen <sandeen@xxxxxxxxxxx> wrote: >> >> >> >> On 10/23/18 3:16 AM, Jan Tulak wrote: >>>> I'm not a huge fan of "fix buffer overflows by making them arbitrarily >>>> bigger" - can we use i.e. snprintf to make sure it won't happen again, >>>> and maybe size them based on ... something, instead of "eh, [5000] is >>>> hopefully enough?" >>> I based the new size on gcc outputs, it said how much it can overflow. >>> But I checked the numbers only once or twice and didn't verify it for >>> other reports. >> >> It's always possible that I'm wrong and I'm missing something. :) >> What did gcc say for the cases I pointed out? > > For the txt[256] -> 300: > > stobj.c: In function ‘stobjstrm_highlight’: > stobj.c:227:55: warning: ‘%s’ directive output may be truncated > writing up to 255 bytes into a region of size between 230 and 231 [-Wformat-truncation=] > snprintf(txt, sizeof(txt), "interrupted: %s, cmdarg: %s", > ^~ > stobj.c:227:2: note: ‘snprintf’ output between 26 and 282 bytes into a > destination of size 256 > snprintf(txt, sizeof(txt), "interrupted: %s, cmdarg: %s", > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > (stobjstrm->st_interrupted == BOOL_TRUE) ? "yes" : "no", > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > stobjstrm->st_cmdarg); > ~~~~~~~~~~~~~~~~~~~~~ > > So, I took the 282, added something, and rounded it to a number that > felt good and wasn't much larger than that. Ok, I don't think that's one of the ones I commented on ;) But let's look at this one. For this string: "interrupted: %s, cmdarg: %s" Treating the "%s" as 0 chars, we have 23 chars + NUL, so that's 24. st_cmdarg is char st_cmdarg[INV_STRLEN] and INV_STRLEN is 128. The "interrupted" string is at most 3 chars. So we have 24+3+128=155 chars max AFAICT, maybe 156 w/ the NUL. What are we missing? Oh. INV_STRLEN is defined differently in different places: #define INV_STRLEN 128 and elsewhere #define INV_STRLEN GLOBAL_HDR_STRING_SZ #define GLOBAL_HDR_STRING_SZ 0x100 Ugh. So, in this case, it would be much more foolproof to not pick a magic number like "300" but instead do something like: txt[INV_STRLEN+32] which ensures we have enough for st_cmdarg[INV_STRLEN], as well as the more obvious/easily-countable literal string lengths in the other partsof the message. That way if somebody changes the INV_STRLEN for any reason, all the code still works. (not sure +32 is sufficient, I didn't count closely, but you get the idea). A little extra space is fine, I'm not saying we should count to the nearest character! :) But if a big part of the length depends an argument with a length that depends on a defined macro, let's use that macro so the code doesn't break again later. -eric
