Skip to content

Fix undefined behaviour in samtools fastq with empty QUAL.#2084

Merged
daviesrob merged 1 commit into
samtools:developfrom
jkbonfield:ubsan
Jul 25, 2024
Merged

Fix undefined behaviour in samtools fastq with empty QUAL.#2084
daviesrob merged 1 commit into
samtools:developfrom
jkbonfield:ubsan

Conversation

@jkbonfield

Copy link
Copy Markdown
Contributor

Incrementing via "qt_end += qt != NULL" will be qt_end+=0 when we have a null qt/qt_end, but NULL+0 is technically undefined.

bam_fastq.c:852:30: runtime error: applying zero offset to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior bam_fastq.c:852:30 in

Easy to just replace with two loops and avoid such cleverness. It's not a major portability issue, but it's nice to have no warnings from ubsan as it makes finding other problems easier.

Incrementing via "qt_end += qt != NULL" will be qt_end+=0 when we have
a null qt/qt_end, but NULL+0 is technically undefined.

    bam_fastq.c:852:30: runtime error: applying zero offset to null pointer
    SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior bam_fastq.c:852:30 in

Easy to just replace with two loops and avoid such cleverness.  It's
not a major portability issue, but it's nice to have no warnings from
ubsan as it makes finding other problems easier.
@daviesrob

Copy link
Copy Markdown
Member

Change looks OK, but is qt_end-qt used later strictly allowed if both are NULL?

@jkbonfield

jkbonfield commented Jul 18, 2024

Copy link
Copy Markdown
Contributor Author

Possibly not, but my aim wasn't so much to be 100% anal on the rules if they're not triggering problems and instead to simply get to a point where -fsanitize=undefined triggers no errors and could become part of the CI system (with appropriate envs to make it fail on error).

If other code subsequently starts to trigger issues, then the CI would notify us and we can fix it, but too much naval gazing isn't really productive when something is working.

@daviesrob daviesrob self-assigned this Jul 25, 2024
@daviesrob daviesrob merged commit d417704 into samtools:develop Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants