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

JSON timestamps are not valid ISO 8601 #2353

Closed
normanr opened this issue Oct 25, 2021 · 5 comments
Closed

JSON timestamps are not valid ISO 8601 #2353

normanr opened this issue Oct 25, 2021 · 5 comments
Labels
Component: mosquitto-clients Status: Completed Nothing further to be done with this issue, it can be closed by the requestor or committer. Type: Bug
Milestone

Comments

@normanr
Copy link

normanr commented Oct 25, 2021

b726e2f added a ISO 8601-like output with a Z (indicating UTC) and %z (offset from UTC). I don't think the Z should be present (also the man page doesn't include the Z in the example output).

Currently json_print does a special workaround after printing microseconds to replace the Z (where snprintf wrote it's null byte). It's probably simplest to just drop the microseconds from the time format, and call strftime a second time to format the timezone after the microseconds have been printed, eg:

char *bufp = buf;
int buf_size = sizeof(buf);
int n;

n = strftime(bufp, buf_size, "%Y-%m-%dT%H:%M:%S.", ti);
bufp += n; buf_size -= n;

n = snprintf(bufp, buf_size, "%06d", ns/1000);
bufp += n; buf_size -= n;

strftime(bufp, buf_size, "%z", ti);

I think it would also make sense to move the strftime/snprintf calls before the #ifdef WITH_CJSON because it can be shared with both parts. Also buf could be made smaller, only the first 32 bytes will ever be used.

ralight added a commit that referenced this issue Oct 27, 2021
Closes #2353. Thanks to Norman Rasmussen.
@ralight
Copy link
Contributor

ralight commented Oct 27, 2021

Thanks @normanr, I agree with your points. I've implemented it slightly differently because it's a tiny fraction more efficient this way.

@ralight ralight added this to the 2.0.13 milestone Oct 27, 2021
@ralight ralight added Component: mosquitto-clients Status: Completed Nothing further to be done with this issue, it can be closed by the requestor or committer. Type: Bug labels Oct 27, 2021
@normanr
Copy link
Author

normanr commented Oct 28, 2021

note: the buffer is still over-sized (100 bytes), it could be smaller (maybe as small as 32 bytes, worth double-checking or giving a small amount of slack).

@normanr
Copy link
Author

normanr commented Oct 28, 2021

some other notes:

  • I'm not sure why the size passed to snprintf is 9. That ends half-way through the timezone identifer.
  • saving and restoring the overwritten char makes an assumption that the formatted string will always be the same length, I guess that's okay. I'm not sure it's worth the run-time efficiency, over having the code be safer. Is it a very hot code path?

ralight added a commit that referenced this issue Nov 15, 2021
Closes #2353. Thanks to Norman Rasmussen.
@ralight
Copy link
Contributor

ralight commented Nov 15, 2021

Yes, the buffer is oversized, I'll drop it but I don't think it's a big deal.

The size passed to snprintf is 9 because the compiler thinks the int could 9 characters long even though we know it will be no more than 6 characters long.

If strftime doesn't give the same length output every time it's a huge bug in strftime.

@normanr
Copy link
Author

normanr commented Nov 16, 2021

These are both just nits, thanks for the fix!

The size passed to snprintf is 9 because the compiler thinks the int could 9 characters long even though we know it will be no more than 6 characters long.

int32 can be 10 characters, so not sure where the 9 comes from.

If strftime doesn't give the same length output every time it's a huge bug in strftime.

y10k problem 😀 but fair point. iso 8601 requires "prior agreement between the sender and the receiver", when the year is outside [0000, 9999], so unlikely to be an issue for a while.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Component: mosquitto-clients Status: Completed Nothing further to be done with this issue, it can be closed by the requestor or committer. Type: Bug
Projects
None yet
Development

No branches or pull requests

2 participants