Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util.c: fix _WIN32 port of strptime #3071

Merged
merged 1 commit into from
Mar 18, 2024
Merged

Conversation

emanuele6
Copy link
Member

@emanuele6 emanuele6 commented Mar 15, 2024

In _WIN32 builds, TIME_MAX needs to be INT32_MAX.

Also fix UB overflow, and useless check in NetBSD's strptime("%s"):

(time_t_value * 10) <= TIME_MAX

is always true since time_t is signed, and TIME_MAX is its maximum value.

And, if it is not optimised out, it causes a signed integer overflow (undefined in standard C) for inputs that start with a sequence of characters between "922337203685477581" and "999999999999999999" (inclusive, 64-bit time_t).

Also, since the check does not do what it is supposed to do, even if we assume that signed integer overflow is defined like for unsigned integers, on builds where time_t is int32_t, and TIME_MAX is INT32_MAX, this will cause strptime("%s") to accept 99999999999 as a valid timestamp, equivalent to 1410065398 (Sun 7 Sep 04:49:58 UTC 2014) instead of rejecting it.
This works because floor(log10(UINT32_MAX)) == floor(log10(INT32_MAX)) in 32-bit.

Noticed thanks to a compiler warning in the windows build CI.

@emanuele6 emanuele6 force-pushed the fixbsd branch 5 times, most recently from 83ba247 to efabe01 Compare March 15, 2024 18:16
@emanuele6
Copy link
Member Author

Now, jq/libjq builds without warnings on the windows CI job

file

@wader
Copy link
Member

wader commented Mar 18, 2024

This was sent upstream as https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=58041 or? wait for that before merge?

@emanuele6
Copy link
Member Author

This was fixed in NetBSD rewriting the loop in a different way; I could port the NetBSD fix to jq instead of using mine.

@wader
Copy link
Member

wader commented Mar 18, 2024

This was fixed in NetBSD rewriting the loop in a different way; I could port the NetBSD fix to jq instead of using mine.

Ok, if equivalent fix i think using the netbsd version will be less confusing, thinking if there will be backports in the future etc

@emanuele6 emanuele6 force-pushed the fixbsd branch 3 times, most recently from 271e10f to efa41d0 Compare March 18, 2024 13:47
@emanuele6
Copy link
Member Author

The NetBSD code triggers another warning:

ake[2]: Entering directory '/d/a/jq/jq'
  CC       src/jv_dtoa_tsd.lo
  CC       src/lexer.lo
  CC       src/parser.lo
  GEN      jq.1
  CC       src/main.o
  CC       src/builtin.lo
  CC       src/bytecode.lo
  CC       src/compile.lo
  CC       src/execute.lo
  CC       src/jq_test.lo
  CC       src/jv.lo
  CC       src/jv_alloc.lo
  CC       src/jv_aux.lo
  CC       src/jv_dtoa.lo
  CC       src/jv_file.lo
  CC       src/jv_parse.lo
  CC       src/jv_print.lo
  CC       src/jv_unicode.lo
  CC       src/linker.lo
  CC       src/locfile.lo
  CC       src/util.lo
  CC       src/decNumber/decContext.lo
  CC       src/decNumber/decNumber.lo
src/util.c: In function 'strptime':
src/util.c:843:25: warning: comparison of integer expressions of different signedness: 'time_t' {aka 'long int'} and 'long unsigned int' [-Wsign-compare]
  843 |                 if (sse > TIME_MAX - d) {
      |                         ^
  CCLD     libjq.la
  CCLD     jq.exe
make[2]: Leaving directory '/d/a/jq/jq'
make[1]: Leaving directory '/d/a/jq/jq'

@wader
Copy link
Member

wader commented Mar 18, 2024

The NetBSD code triggers another warning:

the joy! 🥳

In windows, time_t is a signed 32-bit integer type, so TIME_MAX needs to
be declared as INT32_MAX instead of INT64_MAX.

Also bump NetBSD's strptime to revision 1.65 from 1.63 to fix undefined
behaviour (signed integer overflow) bugs.

Related NetBSD problem report:
https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=58041

Noticed thanks to a compiler warning in the windows build CI.
src/util.c Show resolved Hide resolved
@emanuele6 emanuele6 merged commit d777b65 into jqlang:master Mar 18, 2024
28 checks passed
@emanuele6 emanuele6 deleted the fixbsd branch March 18, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants