Bug 2769 - String truncation warnings in fmt_scaled
Summary: String truncation warnings in fmt_scaled
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: sftp (show other bugs)
Version: -current
Hardware: Other Linux
: P5 normal
Assignee: Damien Miller
URL:
Keywords:
Depends on:
Blocks: V_7_8
  Show dependency tree / graph
 
Reported: 2017-08-26 21:43 AEST by Colin Watson
Modified: 2018-10-19 17:17 AEDT (History)
2 users (show)

See Also:

Attachments
Help compiler prove that fmt_scaled snprintf fits (983 bytes, patch)
2017-08-26 21:43 AEST, Colin Watson 		no flags Details | Diff
with 100% less assert(3) (575 bytes, patch)
2018-04-13 13:54 AEST, Damien Miller 		dtucker: ok+
Details | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Watson 2017-08-26 21:43:31 AEST 
Created attachment 3042 [details]
Help compiler prove that fmt_scaled snprintf fits

fmt_scaled.c: In function ‘fmt_scaled’:
fmt_scaled.c:269:52: warning: ‘%1lld’ directive output may be truncated writing between 1 and 17 bytes into a region of size between 0 and 5 [-Wformat-truncation=]
   (void)snprintf(result, FMT_SCALED_STRSIZE, "%lld.%1lld%c",
                                                    ^~~~~
fmt_scaled.c:269:46: note: directive argument in the range [-9007199254740992, 9007199254740991]
   (void)snprintf(result, FMT_SCALED_STRSIZE, "%lld.%1lld%c",
                                              ^~~~~~~~~~~~~~
In file included from /usr/include/stdio.h:938:0,
                 from /usr/include/bsd/libutil.h:46,
                 from ../includes.h:141,
                 from fmt_scaled.c:41:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:64:10: note: ‘__builtin___snprintf_chk’ output between 5 and 40 bytes into a destination of size 7
   return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        __bos (__s), __fmt, __va_arg_pack ());
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This is actually OK, I think, but the compiler can't quite prove it.  The attached patch helps it do so by syncing up a condition with the nearby comment and by adding an assertion.
Comment 1 Darren Tucker 2017-08-27 10:33:37 AEST 
this function comes from OpenBSD's libutil and I'll have to see what the policy on asserts in that is.
Comment 2 Colin Watson 2017-08-27 16:12:50 AEST 
If you can't do an assertion, then some approach like "if (fract < 0) fall_over;" should have a similar effect in terms of allowing the compiler to deduce correct bounds on fract.
Comment 3 Damien Miller 2018-04-06 13:12:12 AEST 
Move to OpenSSH 7.8 tracking bug
Comment 4 Damien Miller 2018-04-13 13:54:46 AEST 
Created attachment 3141 [details]
with 100% less assert(3)

This one does it without assert, by clamping fract to [0, 10).
Comment 5 Damien Miller 2018-06-01 13:36:49 AEST 
This is fixed in HEAD and will be in OpenSSH 7.8:

commit 32e4e94e1511fe0020fbfbb62399d31b2d22a801
Author: Damien Miller <djm@mindrot.org>
Date:   Mon May 14 14:40:08 2018 +1000

    sync fmt_scaled.c
    
    revision 1.17
    date: 2018/05/14 04:39:04;  author: djm;  state: Exp;  lines: +5 -2;
    commitid: 53zY8GjViUBnWo8Z;
    constrain fractional part to [0-9] (less confusing to static analysis); ok ian@
Comment 6 Damien Miller 2018-10-19 17:17:27 AEDT 
Close RESOLVED bugs with the release of openssh-8.0