Skip to content

Fix GH-15824 mb_detect_encoding() invalid "UTF8" - #15829

Merged
youkidearitai merged 1 commit into
php:masterfrom
youkidearitai:fix-gh-15824
Sep 11, 2024
Merged

Fix GH-15824 mb_detect_encoding() invalid "UTF8"#15829
youkidearitai merged 1 commit into
php:masterfrom
youkidearitai:fix-gh-15824

Conversation

@youkidearitai

@youkidearitaiyoukidearitai commented Sep 10, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

I fixed from strcasecmp to strncasecmp.
However, strncasecmp is specify size to 3rd parameter.
Hence, Add check strlen to mime and aliases.

@youkidearitaiyoukidearitai mentioned this pull request Sep 10, 2024
Closed
ndossche
ndossche requested changes Sep 10, 2024
View reviewed changes
Comment threadext/mbstring/libmbfl/mbfl/mbfl_encoding.c Outdated

@ndosschendosscheSep 10, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This check shouldn't be necessary

@youkidearitaiyoukidearitaiSep 10, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks. Removed.

Comment threadext/mbstring/tests/gh15824.phpt Outdated

@ndosschendosscheSep 10, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add more test cases such that both mime names and aliases are tested.

@youkidearitaiyoukidearitaiSep 10, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Added.

Comment threadext/mbstring/libmbfl/mbfl/mbfl_encoding.c Outdated
Comment threadext/mbstring/libmbfl/mbfl/mbfl_encoding.c Outdated

@ndosschendosscheSep 10, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should use strncasecmp and the same == '\0' check that I suggested below.

@youkidearitaiyoukidearitaiSep 10, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thank you very much for pretty code. Fixed.

@ndosschendossche linked an issue Sep 10, 2024 that may be closed by this pull request
ValueError: mb_detect_encoding(): Argument #2 ($encodings) contains invalid encoding "UTF8" #15824
Closed
ndossche
ndossche reviewed Sep 10, 2024
View reviewed changes
Comment threadext/mbstring/libmbfl/mbfl/mbfl_encoding.c Outdated

@ndosschendosscheSep 10, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You still need strncasecmp here too.

@youkidearitaiyoukidearitaiSep 10, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Nice catch, thanks.

@youkidearitai@ndossche
FixphpGH-15824mb_detect_encoding() invalid "UTF8"
d005317
I fixed from strcasecmp to strncasecmp.
However, strncasecmp is specify size to php#3 parameter.
Hence, Add check length to mime and aliases.
Co-authored-by: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
ndossche
ndossche approved these changes Sep 10, 2024
View reviewed changes

@ndosschendossche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@youkidearitai
youkidearitai merged commit dc5f3b9 into php:masterSep 11, 2024
@youkidearitai
youkidearitai deleted the fix-gh-15824 branch September 11, 2024 00:40
@alexdowad

alexdowad commented Sep 11, 2024

Copy link
Copy Markdown
Contributor

@youkidearitai Indeed, thanks. It seems you are a committer for php-src now? If so, do you want to re-apply 0bcc1e613bc?

@youkidearitai

youkidearitai commented Sep 11, 2024

Copy link
Copy Markdown
ContributorAuthor

@alexdowad Thank you for confirmed. Add my account for CODEOWNERS file is already applied in #14744 . Thank you.

@alexdowad

alexdowad commented Sep 11, 2024

Copy link
Copy Markdown
Contributor

@youkidearitai Thanks very much 👍 In future, it will be appreciated if you can CC me on such changes.

As a suggestion, if you want to list yourself as a 2nd mbstring maintainer in EXTENSIONS, I have no objection to that.

@youkidearitai

youkidearitai commented Sep 11, 2024•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@alexdowad

it will be appreciated if you can CC me on such changes.

Okay. I will do in next time.

As a suggestion, if you want to list yourself as a 2nd mbstring maintainer in EXTENSIONS, I have no objection to that.

Thank you. I will add myself for EXTENSIONS.

@youkidearitaiyoukidearitai mentioned this pull request Sep 13, 2024
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ndosschendosschendossche approved these changes
@alexdowadalexdowadAwaiting requested review from alexdowadalexdowad is a code owner

Assignees

No one assigned

Labels

Extension: mbstring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ValueError: mb_detect_encoding(): Argument #2 ($encodings) contains invalid encoding "UTF8"

3 participants

@youkidearitai@alexdowad@ndossche