From: Pedro Luis Castedo Cepeda <pedroluis.castedo@upm.es>
To: cygwin-patches@cygwin.com
Subject: Re: [PATCH] fix(libc): Fix handle of %E & %O modifiers at end of format string
Date: Fri, 10 Nov 2023 18:44:27 +0100 [thread overview]
Message-ID: <27a7257d-1e06-40ff-89ec-f100b8734802@upm.es> (raw)
In-Reply-To: <ZU4C+UIcYTtvWrrJ@calimero.vinschen.de>
[-- Attachment #1.1: Type: text/plain, Size: 3031 bytes --]
El 10/11/2023 a las 11:16, Corinna Vinschen escribió:
> On Nov 9 23:17, Brian Inglis wrote:
>> On 2023-11-09 12:04, Pedro Luis Castedo Cepeda wrote:
>>> - Prevent strftime to parsing format string beyond its end when
>>> it finish with "%E" or "%O".
>>> ---
>>> newlib/libc/time/strftime.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/newlib/libc/time/strftime.c b/newlib/libc/time/strftime.c
>>> index 56f227c5f..c4e9e45a9 100644
>>> --- a/newlib/libc/time/strftime.c
>>> +++ b/newlib/libc/time/strftime.c
>>> @@ -754,6 +754,8 @@ __strftime (CHAR *s, size_t maxsize, const CHAR *format,
>>> switch (*format)
>>> {
>>> + case CQ('\0'):
>>> + break;
>>> case CQ('a'):
>>> _ctloc (wday[tim_p->tm_wday]);
>>> for (i = 0; i < ctloclen; i++)
>>
>> These cases appear to already be taken care of by setting and using
>> (depending on the config parameters) the "alt" variable for those modifiers,
>> and the default: return 0; for the format *character* (possibly wide) not
>> matching following any modifiers.
>>
>> Patches to newlib should go to the newlib mailing list at sourceware dot org.
>
> Also, a simple reproducer would be nice.
>
>
> Thanks,
> Corinna
My first contribution. Sorry about posting to wrong mail list and, at
best, minimalistic patch motivation reasoning. First time with git
send-mail, too.
I came across this newlib "feature" trying to update GLib port to
2.78.1. When trying to find out why test_strftime (glib/test/date.c)
was failing I discovered that one of the test format strings, "%E" was
triggering a loop in g_date_strftime (glib/gdate.c) requiring more and
more memory till it was stopped by a fortunate maximum size check in
function.
The problem is that __strftime (newlib/libc/time/strftime.c) doesn't
check for '\0' after a terminal "%E" and it continues parsing the format
string. Finally (not sure if intentionally), this triggers a direct
return 0 from __strftime instead breaking the loop, preventing it from
add '\0' to the end of returned string. Same for "%O", I think (not tested).
It seems that this trailing '\0' allows to differentiate returning an
empty string from needing more space (at least, in Glib).
So, is it a newlib bug? Not really, I think this format string is
bad-formed (%E should modify something, shouldn't it?) So undefined
behaviour is OK. I could patch-out these format strings from the port.
But... from Glib tests, it seems that, at least:
- If G_OS_WIN32, terminal "%E" & "%O" are silently discarded.
- If __FreeBSD__ || __OpenBSD__ || __APPLE__ they are transformed to E &
O, respectively.
- And if #else the same thing is expected.
So it seems that returning 0-terminated string is a common practice and
I also think that this is more deterministic and, potentially, safer.
That's why I sent the patch. It tries to be the shortest addition to
check for end of string after %E & %O modifiers and takes G_OS_WIN32
approach (only cause it's the simplest).
Best regards.
[-- Attachment #1.2: strftime_test.c --]
[-- Type: text/plain, Size: 738 bytes --]
#include <stdlib.h>
#include <stdbool.h>
#include <string.h>
#include <time.h>
#include <locale.h>
#include <assert.h>
int main(int argc, char *argv[])
{
const struct tm date = {
.tm_sec = 0, .tm_min = 0, .tm_hour = 0, .tm_mday = 1, .tm_mon = 0,
.tm_year = -1899, .tm_wday = 1, .tm_yday = 0, .tm_isdst = -1,
.tm_gmtoff = 0, .tm_zone = 0x0
};
setenv("LC_ALL", "en_US.utf-8", true);
setlocale (LC_ALL, "");
char tmpbuf[128];
size_t tmplen;
tmpbuf[0] = '\1';
tmplen = strftime(tmpbuf, sizeof(tmpbuf), "%E", &date);
assert(tmpbuf[0] = '\0' || strncmp(tmpbuf, "E", 2));
tmplen = strftime(tmpbuf, sizeof(tmpbuf), "%O", &date);
assert(tmpbuf[0] = '\0' || strncmp(tmpbuf, "O", 2));
return EXIT_SUCCESS;
}
[-- Attachment #2: Firma criptográfica S/MIME --]
[-- Type: application/pkcs7-signature, Size: 5824 bytes --]
next prev parent reply other threads:[~2023-11-10 17:44 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-09 19:04 Pedro Luis Castedo Cepeda
2023-11-10 6:17 ` Brian Inglis
2023-11-10 10:16 ` Corinna Vinschen
2023-11-10 17:44 ` Pedro Luis Castedo Cepeda [this message]
2023-11-11 5:57 ` Brian Inglis
2023-11-11 17:29 ` Pedro Luis Castedo Cepeda
2023-11-13 16:42 ` Corinna Vinschen
2023-11-13 18:46 ` Brian Inglis
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=27a7257d-1e06-40ff-89ec-f100b8734802@upm.es \
--to=pedroluis.castedo@upm.es \
--cc=cygwin-patches@cygwin.com \
/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).