| Message ID | 53E6A799.7020607@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sun, 10 Aug 2014, Chen Gang wrote: > For "[%d]" in sprintf(), theoretically, the maximize size is 14 -- for > 0x80000000, it is sizeof("[-2147483648]"). Normally, it may not cause > real world bug, but if another issues occur, it may lead things worse. Negative values certainly don't make sense here, although it may still make sense to be robust against them. If you get one, it probably means a conversion from HOST_WIDE_INT to int has overflowed. int_size_in_bytes returns HOST_WIDE_INT (64-bit). I think the size variable should be changed to HOST_WIDE_INT, buff expanded to be big enough for any 64-bit value, and HOST_WIDE_INT_PRINT_DEC used in the sprintf format. Separately, I think it's generally a good idea to convert sprintf uses in GCC to snprintf - checking in each case that the size is such that truncation shouldn't occur, but snprintf provides extra safety in case there is some problem with the size calculation (although there may also be a case for having an snprintf variant with an assertion that no truncation occurs, so that the compiler safely aborts rather than either continuing on truncation or overflowing a buffer).
On 08/12/2014 04:25 AM, Joseph S. Myers wrote: > On Sun, 10 Aug 2014, Chen Gang wrote: > >> For "[%d]" in sprintf(), theoretically, the maximize size is 14 -- for >> 0x80000000, it is sizeof("[-2147483648]"). Normally, it may not cause >> real world bug, but if another issues occur, it may lead things worse. > > Negative values certainly don't make sense here, although it may still > make sense to be robust against them. If you get one, it probably means a > conversion from HOST_WIDE_INT to int has overflowed. > > int_size_in_bytes returns HOST_WIDE_INT (64-bit). I think the size > variable should be changed to HOST_WIDE_INT, buff expanded to be big > enough for any 64-bit value, and HOST_WIDE_INT_PRINT_DEC used in the > sprintf format. > OK, thanks, what you said sounds reasonable to me. > Separately, I think it's generally a good idea to convert sprintf uses in > GCC to snprintf - checking in each case that the size is such that > truncation shouldn't occur, but snprintf provides extra safety in case > there is some problem with the size calculation (although there may also > be a case for having an snprintf variant with an assertion that no > truncation occurs, so that the compiler safely aborts rather than either > continuing on truncation or overflowing a buffer). > For me, if buffer is already enough, sprintf() is more obvious than snprintf() which let readers know about it should have no truncation, although what you said about snprintf() is true to me. Welcome additional disccusions or completions, and if no any additional reply within 1 week, I shall send patch v2 for it (still use sprintf). Thanks.
On Aug 11, 2014, at 2:27 PM, Chen Gang <gang.chen.5i5j@gmail.com> wrote: > Welcome additional disccusions or completions, and if no any additional > reply within 1 week, I shall send patch v2 for it (still use sprintf). So, my take is it is easier for a maintainer to re-review if you do it without additional delay. I’d recommend addressing the review points and posting without waiting a week in this case. Waiting is useful if a review point is contentious.
On 8/12/14 7:38, Mike Stump wrote: > On Aug 11, 2014, at 2:27 PM, Chen Gang <gang.chen.5i5j@gmail.com> wrote: >> Welcome additional disccusions or completions, and if no any additional >> reply within 1 week, I shall send patch v2 for it (still use sprintf). > > So, my take is it is easier for a maintainer to re-review if you do it without additional delay. I’d recommend addressing the review points and posting without waiting a week in this case. Waiting is useful if a review point is contentious. > OK, thanks. What you said sounds reasonable to me. But excuse me, I have no enough time resource on it, and also, I am still not quite familiar with building and checking (especially it needs a long time to build and check which has negative effect for analyzing). So, excuse me again, I have to need more time period on it. But I should try send patch v2 for it within this week (2014-08-17). And still welcome additional ideas, suggestions or completions. Thanks.
diff --git a/gcc/c/c-aux-info.c b/gcc/c/c-aux-info.c index 4b6b2d0..b7eacd7 100644 --- a/gcc/c/c-aux-info.c +++ b/gcc/c/c-aux-info.c @@ -311,7 +311,7 @@ gen_type (const char *ret_val, tree t, formals_style style) else { int size = (int_size_in_bytes (t) / int_size_in_bytes (TREE_TYPE (t))); - char buff[10]; + char buff[14]; sprintf (buff, "[%d]", size); ret_val = gen_type (concat (ret_val, buff, NULL), TREE_TYPE (t), style);
For "[%d]" in sprintf(), theoretically, the maximize size is 14 -- for 0x80000000, it is sizeof("[-2147483648]"). Normally, it may not cause real world bug, but if another issues occur, it may lead things worse. It passes "make && make check" under x86_64-apple-darwin13.3.0. 2014-08-09 Chen Gang <gang.chen.5i5j@gmail.com> * c/c-aux-info.c (gen_type): Resize 'buff' from 10 to 14 bytes. --- gcc/c/c-aux-info.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)