[Rd] Out-of-bounds read in R_is_redirection_tty() on Windows (wcsstr over non-NUL-terminated FILE_NAME_INFO.FileName)

[Kai Lin]

Dear R-devel,

I believe there is an out-of-bounds read in R_is_redirection_tty()
(src/main/sysutils.c) on Windows. It intermittently crashes R with a
STATUS_ACCESS_VIOLATION (0xC0000005) whenever one of R's standard
streams is a pipe. Code inspection alone seems sufficient, but I also
have a crash dump that confirms it; details below. The code is unchanged
in current r-devel trunk (observed on R 4.5.2, x86_64-w64-mingw32).

The function runs at startup -- src/gnuwin32/system.c calls
R_is_redirection_tty(0) && R_is_redirection_tty(1) -- and again from
R_isatty(). It returns early unless the handle is FILE_TYPE_PIPE, so the
bug affects only front-ends that connect R's std streams to pipes
(process managers, CI systems, orchestration tools, embedding). Console
and file handles are unaffected.

The problem

GetFileInformationByHandleEx(..., FileNameInfo, ...) fills a
FILE_NAME_INFO whose FileName field is a counted string of
FileNameLength bytes and is NOT NUL-terminated (the documented Win32
contract). The code allocates a buffer sized only for the name, never
writes a terminator, and passes FileName to wcsstr(), which scans until
it finds a NUL. The relevant lines (around sysutils.c:3050):

    DWORD fnLength = fnInfo->FileNameLength;
    size = sizeof(FILE_NAME_INFO) + fnLength;   /* no room for terminator */
    free(fnInfo);
    fnInfo = malloc(size);                       /* uninitialised */
    fnInfo->FileNameLength = fnLength;
    r = GetFileInformationByHandleEx(h, FileNameInfo, fnInfo, size);
    if (r)
        /* note that fnInfo->FileName is not null terminated */
        res = ((wcsstr(fnInfo->FileName, L"msys-") ||
                wcsstr(fnInfo->FileName, L"cygwin-")) &&
                wcsstr(fnInfo->FileName, L"-pty"));

The existing comment already notes FileName is not NUL-terminated, but
wcsstr() requires a terminator. The few slack bytes from struct padding
are uninitialised (malloc, not calloc), so they stop the scan only when
they happen to be zero. When they are non-zero and the buffer sits at
the end of a committed page with the next page unmapped, wcsstr() reads
across the page boundary and the process dies. Hence the crash is
heap-layout dependent and intermittent.

Crash dump (ucrtbase symbols)

    Faulting instruction:  ucrtbase!wcsstr+0x62   movdqu xmm0, [rax]
    rax = ...fdad2000   (a 4 KB page boundary; the page is unmapped)
    rdi = ...fdad1f20   (fnInfo->FileName, readable)
    rdx -> L"msys-"     (the needle from the source)
    caller: R!R_is_redirection_tty

The scan ran ~224 bytes past the buffer base without meeting a NUL and
faulted on the first 16-byte SSE load that crossed into the unmapped
page.

Suggested fix

Reserve one extra WCHAR and terminate the name before searching:

    size = sizeof(FILE_NAME_INFO) + fnLength + sizeof(WCHAR);
    ...
    r = GetFileInformationByHandleEx(h, FileNameInfo, fnInfo, size);
    if (r) {
        /* FileName is a counted string, not NUL-terminated: terminate it */
        fnInfo->FileName[fnInfo->FileNameLength / sizeof(WCHAR)] = L'\0';
        res = ((wcsstr(fnInfo->FileName, L"msys-") ||
                wcsstr(fnInfo->FileName, L"cygwin-")) &&
                wcsstr(fnInfo->FileName, L"-pty"));
    }

A length-bounded search would also work; terminating is the smaller
change and keeps the existing wcsstr() logic. Using FileNameLength from
the second (successful) call for the index guards against the length
changing between the two queries.

As a workaround, front-ends can ensure R's std streams are not pipes
(redirect to files or the NUL device), which skips the affected branch.

Thanks,
Kai Lin, Aaron Jiang, Fable 5, KimiK3
<your name>
<optional affiliation>

Kai Lin
Head of Data Science, CTO
M: +61 422 144 887
E: kai.lin using coolabahcapital.com


Website: coolabahcapital.com<https://coolabahcapital.com/>
Podcast: coolabah.podbean.com<https://coolabah.podbean.com/>
YouTube: Coolabah Capital Investments<https://www.youtube.com/channel/UCU5hkECR7oKSH4HzPjqSNHQ>
[cid:all-logos-inline-740_db850fcf-a816-414d-815f-c1213c11aa12.png]

Australia - Sydney
Australia - Melbourne
United Kingdom - London
United States - Miami
New Zealand - Auckland

Level 3, 1 Bligh Street, Sydney NSW 2000, Australia

Level 38, South Tower, 80 Collins Street, Melbourne VIC 3000, Australia

1st Floor, 10 Foster Lane, London EC2V 6HR, England, United Kingdom

1395 Brickell Avenue, Suite 740, Miami, FL 33131, United States

Level 17, 1 Albert Street, Auckland CBD, Auckland 1010, New Zealand


NOTICES: 1) THIS EMAIL (INCLUDING ATTACHMENTS) IS INTENDED SOLELY FOR THE NAMED ADDRESSEE. THE CONTENTS ARE CONFIDENTIAL, MAY BE PRIVILEGED AND/OR OTHERWISE PROTECTED FROM DISCLOSURE. ANY CONFIDENTIALITY, LEGAL PRIVILEGE AND OTHER PROFESSIONAL PRIVILEGE IS NOT WAIVED, LOST OR DESTROYED BECAUSE THIS EMAIL WAS SENT TO YOU BY MISTAKE. THIS EMAIL IS ALSO SUBJECT TO COPYRIGHT. ANY UNAUTHORISED USE OF ANY PART OF THE CONTENTS IS EXPRESSLY PROHIBITED. IF YOU HAVE RECEIVED THIS EMAIL IN ERROR, PLEASE ADVISE US IMMEDIATELY AND THEN DELETE ALL COPIES OF THIS EMAIL FROM YOUR COMPUTER SYSTEM AND ALL OTHER RECORDS WHETHER ELECTRONIC, HARD COPY OR OTHERWISE. 2) EMAIL TRANSMISSIONS CANNOT BE GUARANTEED TO BE ERROR-FREE OR SECURE. EMAILS MAY BE INCOMPLETE, LOST, CORRUPTED, DESTROYED, INTERCEPTED OR CONTAIN DEFECTS OR VIRUSES. IT IS YOUR RESPONSIBILITY TO CHECK THIS EMAIL FOR DEFECTS OR VIRUSES BEFORE OPENING OR OTHERWISE DEALING WITH IT. PERSONAL INFORMATION IS HANDLED IN ACCORDANCE WITH OUR PRIVACY POLICY, AVAILABLE UPON REQUEST. 3) NEITHER SMARTER MONEY INVESTMENTS PTY LTD, COOLABAH CAPITAL INVESTMENTS PTY LTD NOR ANY ASSOCIATE ('GROUP') MAKE ANY REPRESENTATION OR WARRANTY AS TO WHETHER THE INFORMATION IN THIS EMAIL IS ACCURATE, COMPLETE OR UP TO DATE. TO THE MAXIMUM EXTENT PERMITTED BY LAW, THE GROUP ACCEPT NO RESPONSIBILITY FOR ANY MISSTATEMENTS OR OMISSIONS, NEGLIGENT OR OTHERWISE.