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

Fix crash when parsing timestamps with microsecond text #101

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

wlsdms0122
Copy link

@wlsdms0122 wlsdms0122 commented Nov 2, 2023

Crash occurred even parsing logic enhanced by #72.

When you get rows from the simple query with TIMESTAMP column like this.

id ... created_at
1 ... 1970-01-01 00:00:01.000000

Application crashed at MySQLData.swift:459 due to forced unwrapping of time with a value of nil.

457:            ...
458:            case .datetime, .timestamp:
450:                return (self.time!.date ?? Date(timeIntervalSince1970: 0)).description

If we follow the logic of the MySQLTime conversion of TIMESTAMP text to MySQLTime, we can find that point of crash.

public init?(_ string: String) {
    ...    
    guard parts.count >= 6,
          let year = UInt16(parts[0]),
          let month = UInt16(parts[1]),
          let day = UInt16(parts[2]),
          let hour = UInt16(parts[3]),
          let minute = UInt16(parts[4]),
          let second = UInt16(parts[5]) // <- `parts[5]` is "01.000000" actually. Conversion to `UInt16` fails because of microseconds.
    else {
        return nil
    }
    ...
}

So we added code to parse the real number text correctly.

@0xTim 0xTim requested a review from gwynne November 16, 2023 15:05
@0xTim
Copy link
Member

0xTim commented Mar 18, 2024

@wlsdms0122 I'll leave @gwynne to do the actual review but can you add a test to show it not crashing? Thanks!

@wlsdms0122
Copy link
Author

@0xTim I added a test case. The main branch doesn't pass it.

Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reluctantly approved (I'm always hesitant to so much as breathe too near the disaster that is the date/time logic, but try as I might I can't see anything wrong here 😅).

@0xTim 0xTim added the semver-patch Internal changes only label Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Internal changes only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants