public inbox for newlib-cvs@sourceware.org
help / color / mirror / Atom feed
From: Corinna Vinschen <corinna@sourceware.org>
To: newlib-cvs@sourceware.org
Subject: [newlib-cygwin] svfwscanf: Simplify _sungetwc_r to eliminate apparent buffer overflow
Date: Thu, 19 Aug 2021 10:04:19 +0000 (GMT)	[thread overview]
Message-ID: <20210819100419.3A2153835C3D@sourceware.org> (raw)

https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=cf86601d987b717285a85b2d0315745aa5d63bc8

commit cf86601d987b717285a85b2d0315745aa5d63bc8
Author: Keith Packard <keithp@keithp.com>
Date:   Tue Aug 17 11:51:52 2021 -0700

    svfwscanf: Simplify _sungetwc_r to eliminate apparent buffer overflow
    
    svfwscanf replaces getwc and ungetwc_r. The comments in the code talk
    about avoiding file operations, but they also need to bypass the
    mbtowc calls as svfwscanf operates on wchar_t, not multibyte data,
    which is a more important reason here; they would not work correctly
    otherwise.
    
    The ungetwc replacement has code which uses the 3 byte FILE _ubuf
    field, but if wchar_t is 32-bits, this field is not large enough to
    hold even one wchar_t value. Building in this mode generates warnings
    about array overflow:
    
            In file included from ../../newlib/libc/stdio/svfiwscanf.c:35:
            ../../newlib/libc/stdio/vfwscanf.c: In function '_sungetwc_r.isra':
            ../../newlib/libc/stdio/vfwscanf.c:316:12: warning: array subscript 4294967295 is above array bounds of 'unsigned char[3]' [-Warray-bounds]
              316 |   fp->_p = &fp->_ubuf[sizeof (fp->_ubuf) - sizeof (wchar_t)];
                  |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            In file included from ../../newlib/libc/stdio/stdio.h:46,
                             from ../../newlib/libc/stdio/vfwscanf.c:82,
                             from ../../newlib/libc/stdio/svfiwscanf.c:35:
            ../../newlib/libc/include/sys/reent.h:216:17: note: while referencing '_ubuf'
              216 |   unsigned char _ubuf[3]; /* guarantee an ungetc() buffer */
                  |                 ^~~~~
    
    However, the vfwscanf code *never* ungets data before the start of the
    scanning operation, and *always* ungets data which matches the input
    at that point, so the code always hits the block which backs up over
    the input data and never hits the block which uses the _ubuf field.
    
    In addition, the svfwscanf code will always start with the unget
    buffer empty, so the ungetwc replacement never needs to support an
    unget buffer at all.
    
    Simplify the code by removing support for everything other than
    backing up over the input data, leaving the check to make sure it
    doesn't get underflowed in case the vfscanf code has a bug in it.
    
    Signed-off-by: Keith Packard <keithp@keithp.com>

Diff:
---
 newlib/libc/stdio/vfwscanf.c | 40 +++-------------------------------------
 1 file changed, 3 insertions(+), 37 deletions(-)

diff --git a/newlib/libc/stdio/vfwscanf.c b/newlib/libc/stdio/vfwscanf.c
index f00d41a09..e9e00dfec 100644
--- a/newlib/libc/stdio/vfwscanf.c
+++ b/newlib/libc/stdio/vfwscanf.c
@@ -272,49 +272,15 @@ _sungetwc_r (struct _reent *data,
   /* After ungetc, we won't be at eof anymore */
   fp->_flags &= ~__SEOF;
 
-  /*
-   * If we are in the middle of ungetwc'ing, just continue.
-   * This may require expanding the current ungetc buffer.
+  /* All ungetwc usage in scanf un-gets the current character, so
+   * just back up over the string if we aren't at the start
    */
-
-  if (HASUB (fp))
-    {
-      if (fp->_r >= fp->_ub._size && __submore (data, fp))
-        {
-          return EOF;
-        }
-      fp->_p -= sizeof (wchar_t);
-      *fp->_p = (wchar_t) wc;
-      fp->_r += sizeof (wchar_t);
-      return wc;
-    }
-
-  /*
-   * If we can handle this by simply backing up, do so,
-   * but never replace the original character.
-   * (This makes swscanf() work when scanning `const' data.)
-   */
-
-  if (fp->_bf._base != NULL && fp->_p > fp->_bf._base
-      && ((wchar_t *)fp->_p)[-1] == wc)
+  if (fp->_bf._base != NULL && fp->_p > fp->_bf._base)
     {
       fp->_p -= sizeof (wchar_t);
       fp->_r += sizeof (wchar_t);
-      return wc;
     }
 
-  /*
-   * Create an ungetc buffer.
-   * Initially, we will use the `reserve' buffer.
-   */
-
-  fp->_ur = fp->_r;
-  fp->_up = fp->_p;
-  fp->_ub._base = fp->_ubuf;
-  fp->_ub._size = sizeof (fp->_ubuf);
-  fp->_p = &fp->_ubuf[sizeof (fp->_ubuf) - sizeof (wchar_t)];
-  *(wchar_t *) fp->_p = wc;
-  fp->_r = 2;
   return wc;
 }


                 reply	other threads:[~2021-08-19 10:04 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210819100419.3A2153835C3D@sourceware.org \
    --to=corinna@sourceware.org \
    --cc=newlib-cvs@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).