Bug 173421 - [libc] [patch] strptime() accepts formats that should be rejected
Summary: [libc] [patch] strptime() accepts formats that should be rejected
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: standards (show other bugs)
Version: unspecified
Hardware: Any Any
: Normal Affects Only Me
Assignee: Pedro F. Giffuni
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-06 14:30 UTC by Fabian Keil
Modified: 2014-07-12 18:48 UTC (History)
0 users

See Also:


Attachments
file.txt (3.05 KB, patch)
2012-11-06 14:30 UTC, Fabian Keil
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fabian Keil 2012-11-06 14:30:00 UTC
If two digits are followed by a space in buf, %H lets strptime()
parse the two digits as hours which is correct, but then move the
format pointer to the end, thus ignoring the rest of buf and
reporting a successful parse operation without looking at any
additional format specifiers.
    
This lets the format "%a, %d-%b-%y %H:%M:%S" "match"
"Thursday, 18-Oct-2012 00:11:22", even though the parsing should
fail after %H matched the 12 (because %y only matches the 20).

The resulting time is obviously incorrect.

This affects applications that deal with multiple date variations
by testing several different formats until finding one that is
accepted by strptime(), for example applications that parse dates
in HTTP headers. I noticed the problem while working on Privoxy.

I only confirmed this problem with the %H specifier, but from the
code it looks like other format specifiers are affected as well.

Applications can work around the problem by checking
"%a, %d-%b-%Y %H:%M:%S" before "%a, %d-%b-%y %H:%M:%S",
but this triggers bugs in other strptime() implementations.

The attached patch solves the problem by completely removing the
code that causes the format pointer skipping. This may be incorrect,
but I couldn't think of a scenario where the code is useful, only of
scenarios where it doesn't hurt.

My test case is also handled correctly if the second while()
condition in the removed code is reversed, which might look
reasonable on a first glance, but breaks if %H is followed
by a space and an additional format specifier, in which case
the %H case would move the format pointer to the beginning of
the next format specifier, which would then reject the space
in the buf.

The second patch additionally removes the questionable code for
a couple of other format specifiers, but at least for my use case
the code didn't cause any problems as the skip condition is always
false. Again I couldn't think of a scenario where it's useful,
though.

Fix: Apply the attached patch after confirming that the removed code indeed does nothing useful.

Patch attached with submission follows:
How-To-Repeat: The output of the test case (which I'll submit per mail to keep
the formatting) should be:

Supposedly matching format: %a, %d-%b-%Y %H:%M:%S
Thursday, 18-Oct-2012 00:11:22 GMT -> Thursday, 18-Oct-2012 00:11:22 GMT
ok

Without the patch it's:

Supposedly matching format: %a, %d-%b-%y %H:%M:%S
Thursday, 18-Oct-2012 00:11:22 GMT -> Sunday, 18-Oct-2020 12:00:00 GMT
fail
Comment 1 Pedro F. Giffuni freebsd_committer 2014-06-19 15:37:58 UTC
The change was included as part of r173421
Comment 2 commit-hook freebsd_committer 2014-06-30 14:53:15 UTC
A commit references this bug:

Author: pfg
Date: Mon Jun 30 14:52:41 UTC 2014
New revision: 268043
URL: http://svnweb.freebsd.org/changeset/base/268043

Log:
  MFC	r267627:
  strptime: add support for %t and %n

  Posix strptime() requires support for %t and %n, which were added
  to the illumos port.  Curiously we were skipping white spaces by
  default in most other cases making %t meaningless.

  We now skip spaces in the case of the %e specifier as strftime(3)
  explicitly adds a space for the single digit case.

  Reference:
  http://pubs.opengroup.org/onlinepubs/009695399/functions/strptime.html

  PR:		173421
  Obtained from:	Illumos (Rev. a11c1571b6942161b0186d0588609448066892c2)

Changes:
_U  stable/10/
  stable/10/lib/libc/stdtime/strptime.c
Comment 3 commit-hook freebsd_committer 2014-07-12 18:45:48 UTC
A commit references this bug:

Author: pfg
Date: Sat Jul 12 18:44:47 UTC 2014
New revision: 268574
URL: http://svnweb.freebsd.org/changeset/base/268574

Log:
  MFC	r267627:
  strptime: add support for %t and %n

  Posix strptime() requires support for %t and %n, which were added
  to the illumos port.  Curiously we were skipping white spaces by
  default in most other cases making %t meaningless.

  We now skip spaces in the case of the %e specifier as strftime(3)
  explicitly adds a space for the single digit case.

  Reference:
  http://pubs.opengroup.org/onlinepubs/009695399/functions/strptime.html

  PR:		173421
  Obtained from:	Illumos (Rev. a11c1571b6942161b0186d0588609448066892c2)

Changes:
_U  stable/9/lib/libc/
_U  stable/9/lib/libc/stdtime/
  stable/9/lib/libc/stdtime/strptime.c
Comment 4 Pedro F. Giffuni freebsd_committer 2014-07-12 18:48:57 UTC
Fixed:

FWIW, I noticed that Apple's libc has a similar (but not identical) fix.