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

Fix String.f32 and String.f64 errors with non null terminated strings #4132

Merged
merged 4 commits into from
Jun 11, 2022

Conversation

ergl
Copy link
Member

@ergl ergl commented Jun 6, 2022

These methods rely on strof/strod, which require null-terminated strings. We
were passing our pointer to C without first checking if our string was
null-terminated, which means that C would be able to read past our memory
allocation.

This fixes #1445

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jun 6, 2022
@SeanTAllen
Copy link
Member

Is there any way to test this? I can't think of anything but I wanted to bring it up as a topic.

@ergl ergl added do not merge This PR should not be merged at this time labels Jun 7, 2022
@ergl
Copy link
Member Author

ergl commented Jun 7, 2022

Marking as do not merge for now since the implementation in this PR has a bug related to reallocation. Will fix it soon-ish.

@SeanTAllen It requires some thinking. It is possible, but it requires control over how memory is allocated in order to produce a valid buffer overflow from the C side.

@ergl ergl force-pushed the ergl/fix-1445 branch from 3cfb3a2 to 2525e88 Compare June 7, 2022 12:11
@ergl
Copy link
Member Author

ergl commented Jun 7, 2022

I've added a test and dropped the "We haven't seen errors from this so far, but let's err on the side of caution" from the commit message. Here's a small program that shows the problem with not passing null-terminated strings to C:

actor Main
  new create(env: Env) =>
    (let str1, let str2: String ref) = "1.1".clone().chop(1)
    try
      // Comment the line below to make str1.f32 fail,
      // which means that str1.f32 is looking at the memory from str2
      str2(0)? = 0
      env.out.print(str1.f32()?.string()) // should print "1"
    else
      env.err.print("str1.f32() failed")
    end

@ergl ergl removed the do not merge This PR should not be merged at this time label Jun 7, 2022
@SeanTAllen SeanTAllen added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Jun 7, 2022
@SeanTAllen
Copy link
Member

This needs release notes

@ponylang-main
Copy link
Contributor

Hi @ergl,

The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 4132.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

ergl added 3 commits June 7, 2022 18:44
These methods rely on strof/strod, which require null-terminated strings. We
were passing our pointer to C without first checking if our string was
null-terminated, which means that C would be able to read past our memory
allocation.

This fixes #1445
@ergl ergl force-pushed the ergl/fix-1445 branch from 2525e88 to 245074f Compare June 7, 2022 16:49
@ergl ergl changed the title Fix String.f32 and String.f64 with non-null terminated strings Fix String.f32 and String.f64 errors with non null terminated strings Jun 8, 2022
@SeanTAllen SeanTAllen merged commit 07aa653 into main Jun 11, 2022
@SeanTAllen SeanTAllen deleted the ergl/fix-1445 branch June 11, 2022 12:10
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Jun 11, 2022
github-actions bot pushed a commit that referenced this pull request Jun 11, 2022
github-actions bot pushed a commit that referenced this pull request Jun 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

strtof and related functions need cstring input
4 participants