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

Serializer.convertShort: removed #15

Merged
merged 2 commits into from
Sep 18, 2023
Merged

Conversation

thielema
Copy link
Collaborator

@thielema thielema commented Sep 17, 2023

replaced by 'C.pack . show'

The Haskell foreign import of cshortToString had wrong parameter type Int. It should have been CShort.
But this silently wraps around any integer in a PDF document at 32767.

I tried to replace 'short' by 'long', but this does not solve the problem. In order to prevent silent wrap-around
I would have to add a check and an 'error',
but the check would have to work for all possible sizes of 'Int' and 'CLong' and would certainly require to convert from and to Integer at some point. This is all error-prone and unsafe.
In contrast to that, 'show' is simple and safe.

Fixed bug #14.

replaced by 'C.pack . show'

The Haskell foreign import of cshortToString had wrong parameter type Int.
It should have been CShort.
But this silently wraps around any integer in a PDF document at 32767.

I tried to replace 'short' by 'long', but this does not solve the problem.
In order to prevent silent wrap-around
I would have to add a check and an 'error',
but the check would have to work for all possible sizes of 'Int' and 'CLong'
and would certainly require to convert from and to Integer at some point.
This is all error-prone and unsafe.
In contrast to that, 'show' is simple and safe.
The C function was vulnerable to buffer overflows.
Remove cbits completely.
Copy link
Owner

@hsyl20 hsyl20 left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@hsyl20 hsyl20 merged commit 935038e into hsyl20:master Sep 18, 2023
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