Skip to content

mprintf: do not ignore length modifiers of %o, %x, %X - #15348

Closed
ArtSin wants to merge 2 commits into
curl:masterfrom
ArtSin:mprintf-fix-o-x-length
Closed

mprintf: do not ignore length modifiers of %o, %x, %X#15348
ArtSin wants to merge 2 commits into
curl:masterfrom
ArtSin:mprintf-fix-o-x-length

Conversation

@ArtSin

@ArtSin ArtSin commented Oct 21, 2024

Copy link
Copy Markdown
Contributor

There are uses of %lx and %zx in the codebase, but parsefmt interpreted them as %x.

There are uses of `%lx` and `%zx` in the codebase, but `parsefmt`
interpreted them as `%x`.
@dfandrich

Copy link
Copy Markdown
Contributor

Analysis of PR #15348 at 22423c41:

Test 284 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test 285 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test 980 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test 1049 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test 1093 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test 1094 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test http/test_09_push.py::TestPush::test_09_02_h2_push[0] failed, which has NOT been flaky recently, so there could be a real issue in this PR.

There are more failures, but that's enough from Gha.

Generated by Testclutch

@bagder

bagder commented Oct 21, 2024

Copy link
Copy Markdown
Member

Lovely! Do you think test 557 can be expanded to test %lx and %zx as well ?

@ArtSin

ArtSin commented Oct 22, 2024

Copy link
Copy Markdown
Contributor Author

Lovely! Do you think test 557 can be expanded to test %lx and %zx as well ?

%o, %x, %X are not tested in 557, should I add them? Would one check per int/long + %o/%x/%X combination be enough?

Also %x and %X are treated as unsigned, while %o is not. Should this be fixed as well? I found only two uses of %o, both with unsigned int's.

@bagder

bagder commented Oct 22, 2024

Copy link
Copy Markdown
Member

%o, %x, %X are not tested in 557, should I add them? Would one check per int/long + %o/%x/%X combination be enough?

Please do, and I figure getting just something added there will be a step up from having nothing!

Also %x and %X are treated as unsigned, while %o is not. Should this be fixed as well? I found only two uses of %o, both with unsigned int's.

Sounds appropriate. %o should be considered unsigned!

`%x` and `%X` were already treated as unsigned, but `%o` was not, even though
it was used with unsigned numbers.
@github-actions github-actions Bot added the tests label Oct 22, 2024
@bagder bagder closed this in 7ca164f Oct 24, 2024
bagder pushed a commit that referenced this pull request Oct 24, 2024
`%x` and `%X` were already treated as unsigned, but `%o` was not, even
though it was used with unsigned numbers.

Closes #15348
@bagder

bagder commented Oct 24, 2024

Copy link
Copy Markdown
Member

Thanks!

@ArtSin
ArtSin deleted the mprintf-fix-o-x-length branch October 25, 2024 15:46
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
There are uses of `%lx` and `%zx` in the codebase, but `parsefmt`
interpreted them as `%x`.

Closes curl#15348
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
`%x` and `%X` were already treated as unsigned, but `%o` was not, even
though it was used with unsigned numbers.

Closes curl#15348
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

3 participants