Project

General

Profile

Bug #6961

64-bit octal printf may overflow internal buffer

Added by Kiichi Ozaki over 3 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Normal
Category:
lib - userland libraries
Start date:
2016-05-10
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:

Description

There is a problem with printf family in 64bit libc.

[repro steps]

$ cat print.c
#include <stdio.h>
int main()
{
    printf("%lo", ~0L);
    return 0;
}
$ gcc -m64 print.c
$ ./a.out
777777777777777777777
$ ./a.out | od -Ax -tx1
000000 00 37 37 37 37 37 37 37 37 37 37 37 37 37 37 37
000010 37 37 37 37 37 37
000016

Unexpectedly, first output character is NUL.

additional evidence:

#include <stdio.h>
#include <string.h>

int
main(void)
{
        int i;
        char buf[32];

        memset(buf, 'r', sizeof (buf));
        sprintf(buf + 4, "%lo", ~0L);

        for (i = 0; i < 32; i++) {
                printf("%2d: %2x\n", i, buf[i]);
        }

        return (0);
}

output:

 0: 72
 1: 72
 2: 72
 3: 72
 4:  0
 5: 37
 6: 37
 7: 37
 8: 37
 9: 37
10: 37
11: 37
12: 37
13: 37
14: 37
15: 37
16: 37
17: 37
18: 37
19: 37
20: 37
21: 37
22: 37
23: 37
24: 37
25: 37
26:  0
27: 72
28: 72
29: 72
30: 72
31: 72

[environment]

OpenIndiana (hipster) and maybe other illumos distributions.

[cause]

https://github.com/illumos/illumos-gate/blob/master/usr/src/lib/libc/port/print/doprnt.c#L1213
https://github.com/illumos/illumos-gate/blob/master/usr/src/lib/libc/port/print/print.h#L66

p = bp = buf + MAXLLDIGS;

This allocates MAXLLDIGS (==21) chars, but ~0 (64bit) otcal is 1777777777777777777777, 22 chars.

https://github.com/illumos/illumos-gate/blob/master/usr/src/lib/libc/port/print/doprnt.c#L1237-L1241

Finally,

buf[-1] is '1'
buf[ 0] is '7'
...
buf[20] is '7'

and bp points &buf[-1]. This is out-of-bound error.

https://github.com/illumos/illumos-gate/blob/master/usr/src/lib/libc/port/print/doprnt.c#L2098

Stack layout is maybe below (OpenIndiana, 64bit libc):

%rbp-0xff0  char   *p     (declared at #L457)
...
%rbp-0xfd8  ssize_t n     (declared at #L661)
%rbp-0xfd0  char    buf[] (declared at #L509)
n = p - bp;

Thus n becomes 22 and buf[-1] becomes 0.

[solution]

I think, simply, we should set MAXLLDIGS to 22.

History

#1

Updated by Igor Kozhukhov over 3 years ago

what is your platform and gcc version?

i'm using DilOS with gcc-4.8:

$ gcc-4.8 -m64 -o p p.c

igor@infra-99-018:/myshare/ssd02/builds/dilos-illumos.lx/9$ ./p
1777777777777777777777

igor@infra-99-018:/myshare/ssd02/builds/dilos-illumos.lx/9$ ./p | od -Ax -tx1
000000 31 37 37 37 37 37 37 37 37 37 37 37 37 37 37 37
000010 37 37 37 37 37 37
000016

all fine

#2

Updated by Kiichi Ozaki over 3 years ago

I checked on OpenIndiana hipster-20151003 (and newer) mainly, but I don't know gcc version which built libc.

This problem depends on compiler (the way of making stack layout of _ndoprnt).
Actually, OpenIndiana-151a8 does not reproduce it (maybe using gcc-4.4.x).

#3

Updated by Igor Kozhukhov over 3 years ago

Kiichi Ozaki wrote:

I checked on OpenIndiana hipster-20151003 (and newer) mainly, but I don't know gcc version which built libc.

This problem depends on compiler (the way of making stack layout of _ndoprnt).
Actually, OpenIndiana-151a8 does not reproduce it (maybe using gcc-4.4.x).

gcc-4.4 test based on my DilOS:

igor@infra-99-018:/myshare/ssd02/builds/dilos-illumos.lx/9$ /usr/gcc/4.4/bin/gcc -m64 -o p p.c 
igor@infra-99-018:/myshare/ssd02/builds/dilos-illumos.lx/9$ ./p
1777777777777777777777igor@infra-99-018:/myshare/ssd02/builds/dilos-illumos.lx/9$ 
igor@infra-99-018:/myshare/ssd02/builds/dilos-illumos.lx/9$ ./p | od -Ax -tx1
000000 31 37 37 37 37 37 37 37 37 37 37 37 37 37 37 37
000010 37 37 37 37 37 37
000016

as you can see - all the same with gcc-4.8.
i have my own builds of gcc44 and gcc48 with patches for illumos builds, but patches based on Rich Lowe patches for illumos.

#4

Updated by Kiichi Ozaki over 3 years ago

Oh, The problem is in libc, so it doesn't matter what gcc version building test program (p.c) is.

#5

Updated by Kiichi Ozaki over 3 years ago

At any rate, I think that the current source code (about the value of MAXLLDIGS) is problematic.

#6

Updated by Igor Kozhukhov over 3 years ago

Kiichi Ozaki wrote:

At any rate, I think that the current source code (about the value of MAXLLDIGS) is problematic.

but i have no problems on my dilos env :)
but i'm using gcc-4.8 for dilos-illumos builds

#7

Updated by Josef Sipek over 3 years ago

Yeah, OI Hipster seems to be broken. As others pointed out, this appears to be an issue with libc and not the compiler used to compile the test program.

jeffpc@meili:/tmp$ clang -m64 -o test-clang print.c
jeffpc@meili:/tmp$ gcc -m64 -o test-gcc print.c  
jeffpc@meili:/tmp$ ./test-gcc | xxd                
0000000: 0037 3737 3737 3737 3737 3737 3737 3737  .777777777777777
0000010: 3737 3737 3737                           777777
jeffpc@meili:/tmp$ ./test-clang | xxd
0000000: 0037 3737 3737 3737 3737 3737 3737 3737  .777777777777777
0000010: 3737 3737 3737                           777777
#8

Updated by Igor Kozhukhov over 3 years ago

OK :)

my point was to:
[environment]
OpenIndiana (hipster) and other illumos distributions.

if you say about 'and other illumos distributions' - you need provide result with distribution name :)
for now - it confirmed on OI, but i can't confirm with DilOS
will wait others distributions

#9

Updated by Josef Sipek over 3 years ago

I just tried on OmniOS 018 - also broken.

I'm guessing that it is likely related to gcc 4.4.4 which most illumos distros use.

#10

Updated by Kiichi Ozaki over 3 years ago

What I mean is, as wrote at ticket description, that there is an out-of-bounds bug in illumos libc code whether incorrect output reproduces or not maybe because of compiler.

Finally,

buf[-1] is '1'
buf[ 0] is '7'
...
buf[20] is '7'

and bp points &buf[-1].

#11

Updated by Robert Mustacchi over 3 years ago

Thank you for reporting this Kiichi. Your analysis here looks correct and that we have undersized the buffer and are accessing this out of bounds it appears. It's quite obvious especially if you modify your second example to start several bytes in.

#include <stdio.h>
#include <string.h>

int
main(void)
{
        int i;
        char buf[32];

        memset(buf, 'r', sizeof (buf));
        sprintf(buf + 4, "%lo", ~0L);

        for (i = 0; i < 32; i++) {
                printf("%d: %x\n", i, buf[i]);
        }

        return (0);
}

Kiichi, do you want to prepare a change for this and some tests or should I take care of that?

#12

Updated by Kiichi Ozaki over 3 years ago

Thank you, but I'm not able to edit description. May I ask?

#13

Updated by Robert Mustacchi over 3 years ago

Kiichi, sorry I missed your comment here. You should be able to now.

#14

Updated by Kiichi Ozaki over 3 years ago

  • Description updated (diff)
#15

Updated by Robert Mustacchi almost 3 years ago

  • Assignee set to Robert Mustacchi
  • % Done changed from 0 to 90
  • Tags deleted (needs-triage)
#16

Updated by Robert Mustacchi over 2 years ago

  • Subject changed from Incorrect octal output of printf family in 64bit libc to 64-bit octal printf may overflow output buffer
  • Status changed from New to Pending RTI
  • % Done changed from 90 to 100
#17

Updated by Robert Mustacchi over 2 years ago

  • Subject changed from 64-bit octal printf may overflow output buffer to 64-bit octal printf may overflow internal buffer
#18

Updated by Electric Monk over 2 years ago

  • Status changed from Pending RTI to Closed

git commit afc62b4b94eec9d9cec1ba14fd65fcf304325e7f

commit  afc62b4b94eec9d9cec1ba14fd65fcf304325e7f
Author: Robert Mustacchi <rm@joyent.com>
Date:   2017-05-30T18:08:04.000Z

    6961 64-bit octal printf may overflow internal buffer
    Reviewed by: Toomas Soome <tsoome@me.com>
    Reviewed by: Yuri Pankov <yuri.pankov@gmail.com>
    Approved by: Dan McDonald <danmcd@kebe.com>

Also available in: Atom PDF