Test expiration date #1233
Test expiration date #1233
Conversation
|
@alejandro-colomar this is the testing for #1217. It contains all your commits there + the implementation to run In addition, one of the test cases ( Finally, does the aforementioned PR also change the date behaviour for useradd and chage? If so, I'll add additional tests for them. |
I think I commented on the one you're mentioning. Was it that one that you were referring to?
It affects all of these (since I'm changing strtoday()): |
ikerexxe
force-pushed
the
test-expiration-date
branch
2 times, most recently
from
9ea12c2 to
8a901b3
Compare
|
@alejandro-colomar do you know why alpine fails when useradd/usermod/chage set the expiration date as YYYY-MM-DD? They all fail even though the date is valid (i.e. 2025-01-01). |
Is it a regression in my patches or is it a failure in master? Could you please open a PR without my changes, to test the tests against master? |
|
It's also failing without your patches. You can set up an alpine container and check it there. Can this be related to the shell they are using? If I'm being honest my knowledge in that area is very limited. |
I don't know. It might be related to the signedness of time_t, or one of those little details. I'll check this weekend when I'm back home and have better internet to create the docker container and install all the deps. |
ikerexxe
force-pushed
the
test-expiration-date
branch
from
8a901b3 to
3badae8
Compare
@alejandro-colomar did you have time to look at this? |
I forgot. I'll try to do it today. |
I can't reproduce it. I've run an /shadow # ./src/useradd foo
/shadow # getent shadow foo
foo:!:20174::::::
/shadow # ./src/usermod -e '1970-10-20' foo
/shadow # getent shadow foo
foo:!:20174:::::292:
/shadow # ./src/usermod -e '1970-10-10' foo
/shadow # getent shadow foo
foo:!:20174:::::282:
/shadow # ./src/usermod -e '2025-01-01' foo
/shadow # getent shadow foo
foo:!:20174:::::20089:
/shadow # ./src/usermod -e '2125-01-01' foo
/shadow # getent shadow foo
foo:!:20174:::::56613:It seems to be working as expected. |
|
Did you setup everything manually? I'm building the container using the automation: |
|
It fails even with the default shell: |
Yep, everything manual. I'll try again with ansible. |
With ansible, it also works for me: $ podman exec -ti builder bash
WARN[0000] Using cgroups-v1 which is deprecated in favor of cgroups-v2 with Podman v5 and will be removed in a future version. Set environment variable `PODMAN_IGNORE_CGROUPSV1_WARNING` to hide this warning.
44da80a259d4:/# cat /etc/os-release
NAME="Alpine Linux"
ID=alpine
VERSION_ID=3.21.3
PRETTY_NAME="Alpine Linux v3.21"
HOME_URL="https://alpinelinux.org/"
BUG_REPORT_URL="https://gitlab.alpinelinux.org/alpine/aports/-/issues"
44da80a259d4:/# ls
bin etc lib mnt proc run srv tmp var
dev home media opt root sbin sys usr
44da80a259d4:/# useradd tt
44da80a259d4:/# getent shadow tt
tt:!:20175:0:99999:7:::
44da80a259d4:/# usermod -e 10 tt
44da80a259d4:/# getent shadow tt
tt:!:20175:0:99999:7::10:
44da80a259d4:/# usermod -e 1970-10-20 tt
44da80a259d4:/# getent shadow tt
tt:!:20175:0:99999:7::292:
44da80a259d4:/# The bad thing about using ansible is that debugging becomes more difficult. I really preferred the old Dockerfiles. |
|
Ok, I've been able to reproduce the problem in a basic Alpine Linux container (no need to use ansible or build shadow) with the following reproducer: This code mimics in a very basic way get_date() function and it should return |
musl seems to have a proper strptime(3), from what I can see in the source code. It must be an issue in Alpine, not musl. Detailsalx@devuan:~/src/musl/libc/master$ grepc strptime .
./include/time.h:char *strptime (const char *__restrict, const char *__restrict, struct tm *__restrict);
./src/time/strptime.c:char *strptime(const char *restrict s, const char *restrict f, struct tm *restrict tm)
{
int i, w, neg, adj, min, range, *dest, dummy;
const char *ex;
size_t len;
int want_century = 0, century = 0, relyear = 0;
while (*f) {
if (*f != '%') {
if (isspace(*f)) for (; *s && isspace(*s); s++);
else if (*s != *f) return 0;
else s++;
f++;
continue;
}
f++;
if (*f == '+') f++;
if (isdigit(*f)) {
char *new_f;
w=strtoul(f, &new_f, 10);
f = new_f;
} else {
w=-1;
}
adj=0;
switch (*f++) {
case 'a': case 'A':
dest = &tm->tm_wday;
min = ABDAY_1;
range = 7;
goto symbolic_range;
case 'b': case 'B': case 'h':
dest = &tm->tm_mon;
min = ABMON_1;
range = 12;
goto symbolic_range;
case 'c':
s = strptime(s, nl_langinfo(D_T_FMT), tm);
if (!s) return 0;
break;
case 'C':
dest = ¢ury;
if (w<0) w=2;
want_century |= 2;
goto numeric_digits;
case 'd': case 'e':
dest = &tm->tm_mday;
min = 1;
range = 31;
goto numeric_range;
case 'D':
s = strptime(s, "%m/%d/%y", tm);
if (!s) return 0;
break;
case 'F':
/* Use temp buffer to implement the odd requirement
* that entire field be width-limited but the year
* subfield not itself be limited. */
i = 0;
char tmp[20];
if (*s == '-' || *s == '+') tmp[i++] = *s++;
while (*s=='0' && isdigit(s[1])) s++;
for (; *s && i<(size_t)w && i+1<sizeof tmp; i++) {
tmp[i] = *s++;
}
tmp[i] = 0;
char *p = strptime(tmp, "%12Y-%m-%d", tm);
if (!p) return 0;
s -= tmp+i-p;
break;
case 'H':
dest = &tm->tm_hour;
min = 0;
range = 24;
goto numeric_range;
case 'I':
dest = &tm->tm_hour;
min = 1;
range = 12;
goto numeric_range;
case 'j':
dest = &tm->tm_yday;
min = 1;
range = 366;
adj = 1;
goto numeric_range;
case 'm':
dest = &tm->tm_mon;
min = 1;
range = 12;
adj = 1;
goto numeric_range;
case 'M':
dest = &tm->tm_min;
min = 0;
range = 60;
goto numeric_range;
case 'n': case 't':
for (; *s && isspace(*s); s++);
break;
case 'p':
ex = nl_langinfo(AM_STR);
len = strlen(ex);
if (!strncasecmp(s, ex, len)) {
tm->tm_hour %= 12;
s += len;
break;
}
ex = nl_langinfo(PM_STR);
len = strlen(ex);
if (!strncasecmp(s, ex, len)) {
tm->tm_hour %= 12;
tm->tm_hour += 12;
s += len;
break;
}
return 0;
case 'r':
s = strptime(s, nl_langinfo(T_FMT_AMPM), tm);
if (!s) return 0;
break;
case 'R':
s = strptime(s, "%H:%M", tm);
if (!s) return 0;
break;
case 's':
/* Parse only. Effect on tm is unspecified
* and presently no effect is implemented.. */
if (*s == '-') s++;
if (!isdigit(*s)) return 0;
while (isdigit(*s)) s++;
break;
case 'S':
dest = &tm->tm_sec;
min = 0;
range = 61;
goto numeric_range;
case 'T':
s = strptime(s, "%H:%M:%S", tm);
if (!s) return 0;
break;
case 'U':
case 'W':
/* Throw away result of %U, %V, %W, %g, and %G. Effect
* is unspecified and there is no clear right choice. */
dest = &dummy;
min = 0;
range = 54;
goto numeric_range;
case 'V':
dest = &dummy;
min = 1;
range = 53;
goto numeric_range;
case 'g':
dest = &dummy;
w = 2;
goto numeric_digits;
case 'G':
dest = &dummy;
if (w<0) w=4;
goto numeric_digits;
case 'u':
dest = &tm->tm_wday;
min = 1;
range = 7;
goto numeric_range;
case 'w':
dest = &tm->tm_wday;
min = 0;
range = 7;
goto numeric_range;
case 'x':
s = strptime(s, nl_langinfo(D_FMT), tm);
if (!s) return 0;
break;
case 'X':
s = strptime(s, nl_langinfo(T_FMT), tm);
if (!s) return 0;
break;
case 'y':
dest = &relyear;
w = 2;
want_century |= 1;
goto numeric_digits;
case 'Y':
dest = &tm->tm_year;
if (w<0) w=4;
adj = 1900;
want_century = 0;
goto numeric_digits;
case 'z':
if (*s == '+') neg = 0;
else if (*s == '-') neg = 1;
else return 0;
for (i=0; i<4; i++) if (!isdigit(s[1+i])) return 0;
tm->__tm_gmtoff = (s[1]-'0')*36000+(s[2]-'0')*3600
+ (s[3]-'0')*600 + (s[4]-'0')*60;
if (neg) tm->__tm_gmtoff = -tm->__tm_gmtoff;
s += 5;
break;
case 'Z':
if (!strncmp(s, tzname[0], len = strlen(tzname[0]))) {
tm->tm_isdst = 0;
s += len;
} else if (!strncmp(s, tzname[1], len=strlen(tzname[1]))) {
tm->tm_isdst = 1;
s += len;
} else {
/* FIXME: is this supposed to be an error? */
while ((*s|32)-'a' <= 'z'-'a') s++;
}
break;
case '%':
if (*s++ != '%') return 0;
break;
default:
return 0;
numeric_range:
if (!isdigit(*s)) return 0;
*dest = 0;
for (i=1; i<=min+range && isdigit(*s); i*=10)
*dest = *dest * 10 + *s++ - '0';
if (*dest - min >= (unsigned)range) return 0;
*dest -= adj;
switch((char *)dest - (char *)tm) {
case offsetof(struct tm, tm_yday):
;
}
goto update;
numeric_digits:
neg = 0;
if (*s == '+') s++;
else if (*s == '-') neg=1, s++;
if (!isdigit(*s)) return 0;
for (*dest=i=0; i<w && isdigit(*s); i++)
*dest = *dest * 10 + *s++ - '0';
if (neg) *dest = -*dest;
*dest -= adj;
goto update;
symbolic_range:
for (i=2*range-1; i>=0; i--) {
ex = nl_langinfo(min+i);
len = strlen(ex);
if (strncasecmp(s, ex, len)) continue;
s += len;
*dest = i % range;
break;
}
if (i<0) return 0;
goto update;
update:
//FIXME
;
}
}
if (want_century) {
tm->tm_year = relyear;
if (want_century & 2) tm->tm_year += century * 100 - 1900;
else if (tm->tm_year <= 68) tm->tm_year += 100;
}
return (char *)s;
} |
Sorry, my bad. Actually, musl has a bug. It doesn't support Cc: @richfelker |
ikerexxe
force-pushed
the
test-expiration-date
branch
from
3badae8 to
926fc1a
Compare
|
@alejandro-colomar your latest updates have partially fixed the problem with dates in ISO 8601 format. Right now in Alpine (with musl) dates that are not correct, for example 2025-18-18 or tomorrow, do not return an error when they should. |
How can I reproduce that in Alpine? I'm getting an error even for valid dates. / # cat strptime.c
#define _XOPEN_SOURCE
#include <stdio.h>
#include <time.h>
int
main(void)
{
struct tm tm;
const char *p;
p = strptime("2025-01-18", "%F", &tm);
if (p == NULL)
perror("A");
else if (*p != 0)
perror("B");
return 0;
}
/ # gcc -Wall -Wextra strptime.c
/ # ./a.out
A: No error information/ # cat /etc/os-release
NAME="Alpine Linux"
ID=alpine
VERSION_ID=3.21.3
PRETTY_NAME="Alpine Linux v3.21"
HOME_URL="https://alpinelinux.org/"
BUG_REPORT_URL="https://gitlab.alpinelinux.org/alpine/aports/-/issues" |
Hmmm, I've tried replacing |
|
So, now that I can work with strptime(3) in Alpine, I've tried this: / # cat strptime.c
#define _XOPEN_SOURCE
#include <stdio.h>
#include <time.h>
int
main(void)
{
struct tm tm;
const char *p;
p = strptime("2025-01-18", "%Y-%m-%d", &tm);
if (p == NULL)
perror("1A");
else if (*p != 0)
perror("1B");
p = strptime("2025-11-18", "%Y-%m-%d", &tm);
if (p == NULL)
perror("2A");
else if (*p != 0)
perror("2B");
p = strptime("2025-18-18", "%Y-%m-%d", &tm);
if (p == NULL)
perror("3A");
else if (*p != 0)
perror("3B");
return 0;
}
/ # gcc -Wall -Wextra strptime.c
/ # ./a.out
3A: No error informationIt seems invalid dates are correctly reported, aren't they? It seems consistent with glibc. @ikerexxe can you reproduce the invalid date bug you're reporting with a simple program that can be run in a docker container? |
I have also replaced |
Thanks! I've fixed that in my PR. Please check now. |
ikerexxe
force-pushed
the
test-expiration-date
branch
from
926fc1a to
421db6e
Compare
|
Good! That solved all issues and now CI is green |
Yes, I'm very happy with those changes and the tests. It seems like everything works correctly and we'll not face any regression, which was my greatest fear.
Np, I'll ask a QE colleague to review them. @danlavu do you mind reviewing this PR? It's based on top of #1217, so you just need to review my changes. In addition, Dominika is updating the documentation at #1243. You can use that as a guide to understand the test cases I'm implementing, and whether I'm missing some scenario. If there is something that does not fit, either in the tests or in the documentation, just tell us and we will take a look at it. |
ikerexxe
force-pushed
the
test-expiration-date
branch
from
b33d07e to
278824e
Compare
|
Once @danlavu reviews your tests, I propose the following:
Does that sound good? |
|
Yeah, that makes perfect sense. |
danlavu
left a comment
danlavu
left a comment
There was a problem hiding this comment.
I only have optional nitpicks. This is because I'm unfamiliar with shadow utilities. These suggestions would have helped me understand the tests faster. Anytime I paused looking at the code, I made a comment. I'm digging deep to find any comments. Everything looks great.
|
By the way, @danlavu what do you think about the test themselves? Do you think we are checking all possibilities regarding the input/output values? |
|
@ikerexxe, the tests are good. Though I never like negative test cases, the I would remove the parametrized values of |
I prefer keeping them.
And since |
I'm not a great fan either, but taking the code history into account they make sense for these scenarios.
This is part of the history I mention, another one is that we are moving from yacc code to C code and we don't have the expertise to fully understand what the yacc code did 100%. It's a bit complex, so I prefer to be cautious and add extra test scenarios. |
ikerexxe
force-pushed
the
test-expiration-date
branch
from
278824e to
097381c
Compare
There was a problem hiding this comment.
I don't know if @hallyn wants to review or comment anything. I'm fine with merging the tests already.
Disclaimer: I can't read python, so I haven't reviewed much, other than having a quick look at it.
I approve the nomenclature of the functions, which is the only thing I feel qualified to review. :)
From @danlavu 's comments, it seems the tests are okay, so I'll approve so that you can merge if you want.
Please remember to remove my commits before merging.
ikerexxe
force-pushed
the
test-expiration-date
branch
from
097381c to
bb8d162
Compare
|
Thank you! I've removed your changes and I'll let CI run. Once this is done I'll proceed to merge. This is my last message until next week, then we can proceed to review and merge #1217 |
|
Ok, some of the invalid dates aren't recognized as such without Alejandro's changes. I don't want to merge and leave CI broken for days, so I'll wait until Monday before proceeding. @alejandro-colomar will you be available on Monday to work on the pending PR? This way CI won't be broken for a long time. |
Yeah, I think mine is ready, so as soon as you or Serge approve it, we can merge it; there's not much work we need to do there. But yeah, I'll be available on Monday (until 16:00 UTC more or less, and then again available in the night). |
Could you please mention which tests don't pass, and why? It would be good to be explicit about what is changing with my code. |
|
@alejandro-colomar the tests are good, all my comments are because I'm missing context. Doc strings accurately describe what the code is doing, code is easy to comprehend and will be easy to maintain and troubleshoot. With the discussion, the scenarios make more sense now. |
|
Failed test cases are related to invalid dates that test dates outside the limits:
IMO, the fortifications added at #1217 improve the code. |
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com> Reviewed-by: Dan Lavu <dlavu@redhat.com>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com> Reviewed-by: Dan Lavu <dlavu@redhat.com>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com> Reviewed-by: Dan Lavu <dlavu@redhat.com>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com> Reviewed-by: Dan Lavu <dlavu@redhat.com>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com> Reviewed-by: Dan Lavu <dlavu@redhat.com>
ikerexxe
force-pushed
the
test-expiration-date
branch
from
bb8d162 to
ae36438
Compare
|
Since everyone agrees and the CI is green I will proceed to merge this PR |
Ahhh, that makes sense. It's because the yacc(1) code was passing the dates to mktime(3), which accepts that kind of thing, unlike strptime(3). |
3 participants
Testing for #1217