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

Use search regex instead of match for metadata check. #369

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

mgautierfr
Copy link
Collaborator

@mgautierfr mgautierfr commented Sep 1, 2023

Fix #345
Fix #352

Investigating #345, it seems that zimcheck-test is getting a segfault (when run with qemu-aarch64) in std::regex_match and the PNG_REGEXP_DATA. It appears the segfault happens with a huge callstack. I don't know where the bug is exactly (qemu ? std ?) but the behavior is the same than #352.

By using regex_search we allow the regex to not match the whole content (but still allow it if the regex ends with $)

As we don't need to check all the PNG data, and check only the beginning, regex_search is passing without segfault (early exit ?)

This PR take another approach than #358 by fixing the regex instead of avoiding it (but it doesn't check for the size)
(To be honest, I've made this fix before realizing that it was fixing the same bug than #352)

@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Patch coverage: 33.33% and no project coverage change.

Comparison is base (dc703e0) 27.72% compared to head (b0a2e0f) 27.72%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #369   +/-   ##
=======================================
  Coverage   27.72%   27.72%           
=======================================
  Files          26       26           
  Lines        2528     2528           
  Branches     1349     1349           
=======================================
  Hits          701      701           
  Misses       1346     1346           
  Partials      481      481           
Files Changed Coverage Δ
src/metadata.cpp 59.74% <33.33%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

LGTM, but a small clean-up would be nice.

Also please add a comment in src/metadata_constraints.cpp stating that the regular expressions must include the ^ and/or $ markers as needed.

src/metadata.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

LGTM. Squash the fixup commit and merge.

Fix #345, #352

Investigating #345, it seems that zimcheck-test is getting a segfault
(when run with qemu-aarch64) in `std::regex_match` and the PNG_REGEXP_DATA.
It appears the segfault happens with a huge callstack.
I don't know where the bug is exactly (qemu ? std ?) but the behavior is
the same than #352.

By using `regex_search` we allow the regex to not match the whole content
(but still allow it if the regex ends with `$`)

As we don't need to check all the PNG data, and check only the beginning,
`regex_search` is passing without segfault (early exit ?)
@mgautierfr mgautierfr merged commit 7ffb64a into main Sep 22, 2023
10 of 13 checks passed
@mgautierfr mgautierfr deleted the fix_aarch64_test branch September 22, 2023 11:52
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.

Large --illustration image size lead to SIGSEGV Fails to build on PPA for arm64
2 participants