Fix issue where string references aren't always completed with a null terminator. #299

charleskorn · 2022-02-19T03:56:02Z

The existing implementation works most of the time because most characters don't consume the full four bytes a single UTF-8 character could consume. An empty string is a good example of where this falls down.

The tests rely on the fact that two consecutive memory allocations usually end up next to each other. This happens reliably for me on my machine, but isn't guaranteed. If there's a better way to test this, I'd love to hear it.

Resolves #276.

… terminator. This is particularly easy to reproduce with an empty string - the existing implementation works most of the time because most characters don't consume the full four bytes a single UTF-8 character could consume. The tests relies on the fact that two consecutive memory allocations usually end up next to each other. This happens reliably for me on my machine, but isn't guaranteed. If there's a better way to test this, I'd love to hear it.

charleskorn · 2022-03-02T22:38:53Z

Any chance you could take a look at this @headius?

headius · 2022-03-03T21:17:30Z

Sorry for the delay! I missed this PR during my periodic reviews of the projects I maintain. I will review and get it merged and you probably would like a release as well, right?

charleskorn · 2022-03-03T21:18:25Z

Sorry for the delay! I missed this PR during my periodic reviews of the projects I maintain. I will review and get it merged and you probably would like a release as well, right?

No worries at all, thanks for taking a look.

A release would be great as well!

headius · 2022-03-03T21:38:01Z

So this is a good incremental fix but you may have noticed there's other issues reported when the target string encoding is not single byte compatible. I would be comfortable merging this for now so the issue is fixed for single byte strings, but if you want to take it to the next level you could try to handle multi-byte encodings as well.

headius · 2022-03-03T21:39:37Z

For example, a dumb fix would be to allocate an extra four null bytes for the string since that would cover all known encodings. We can probably do better than that.

headius · 2022-03-03T21:45:55Z

I went ahead and merged this for now. If you think you might try to tackle the multi-byte issue let me know, otherwise I can spin a release pretty much anytime.

charleskorn · 2022-03-03T21:57:07Z

For example, a dumb fix would be to allocate an extra four null bytes for the string since that would cover all known encodings. We can probably do better than that.

Could you expand on what you had in mind for this?

charleskorn · 2022-03-03T21:57:47Z

I went ahead and merged this for now. If you think you might try to tackle the multi-byte issue let me know, otherwise I can spin a release pretty much anytime.

It'd be great to get this released as this is blocking something I'm working on. I'm happy to take a look at the multi-byte issue separately.

headius · 2022-03-05T19:48:24Z

I can do a quick release of this today. If you need any of the downstream libraries released to have versions match, let me know but for now I will just release jnr-ffi.

charleskorn · 2022-03-06T00:18:28Z

If you need any of the downstream libraries released to have versions match, let me know but for now I will just release jnr-ffi.

A new version of jnr-ffi should be enough. Thanks again for your help @headius!

charleskorn · 2022-03-08T06:16:25Z

Did the release for this go out @headius? I can't see the new version anywhere.

headius · 2022-03-10T14:03:35Z

I'm sorry about the delay... the day I was planning to release I left on a trip and so it did not happen. I'm releasing now!

headius · 2022-03-10T14:10:47Z

Hmm oddly enough I have two failures on Apple x86 with this change:

[ERROR] Failures: 
[ERROR]   PointerNumericTest.testPointerGetBoolean:532 Incorrect boolean value at offset 256 ==> expected: <false> but was: <true>
[ERROR]   PointerNumericTest.testPointerSetBoolean:564 Incorrect boolean value at offset 258 ==> expected: <false> but was: <true>

We need to investigate why these failed before I can release.

headius · 2022-03-10T14:24:09Z

I added a macos-latest job to the CI matrix, and it does indeed show the failures:

https://github.com/jnr/jnr-ffi/runs/5497161286?check_suite_focus=true

I won't have time to investigate this myself today.

charleskorn · 2022-03-12T23:00:55Z

Were those tests passing before this change was introduced? I can't see how this change could impact those two tests.

charleskorn · 2022-03-20T06:38:39Z

Hmm oddly enough I have two failures on Apple x86 with this change:
[ERROR] Failures: 
[ERROR]   PointerNumericTest.testPointerGetBoolean:532 Incorrect boolean value at offset 256 ==> expected: <false> but was: <true>
[ERROR]   PointerNumericTest.testPointerSetBoolean:564 Incorrect boolean value at offset 258 ==> expected: <false> but was: <true>
We need to investigate why these failed before I can release.

Looks like these tests were introduced in 43aa31e / #293, and they fail for me if I check out that commit - ie. they've been broken since they were introduced.

charleskorn · 2022-03-20T07:57:42Z

I've just had a look at the tests myself, but I don't know enough about what the tests are trying to check nor how everything hangs together to know how to go about fixing this issue sorry.

Do you have any suggestions on where to start? Or perhaps @basshelal (as the author of #293) might have some ideas?

basshelal · 2022-03-20T13:37:21Z

@charleskorn Just saw this now and tested it on my x86_64 Macbook, yes it seems something isn't working with that test since its introduction, unrelated to your PR.

I'll have a look and see what I can do about it, the frustrations of cross-platform compatibility never end.

@headius these errors are from my initial commit that introduced them not this PR, I think you can ignore those errors by @Disabled-ing the failing tests, namely:

    @Disabled    
    @Test
    public void testPointerGetBoolean() {...}

    @Disabled
    @Test
    public void testPointerSetBoolean() {...}

I'll have to figure out why these tests fail only on macOS at a later time, though all other tests pass so it's likely either a mistake on my part (though why only on macOS and not GNU/Linux) or a weird quirk with macOS that will need investigating, I'll create an issue now for this.

Thanks @charleskorn for your contributions!

charleskorn · 2022-03-20T19:52:34Z

Thanks for the prompt response @basshelal.

These tests pass on my M1 Mac, but fail on my old Intel Mac - not sure if that's a helpful clue.

basshelal · 2022-03-20T20:20:43Z

@charleskorn wow, even more cross-platform strangeness! This sounds like it'll be a fun big to hunt 🙂

Thanks for this information though

headius · 2022-03-22T16:50:54Z

Ok I will disable those tests and hope for the best?

basshelal · 2022-03-22T16:55:30Z

@headius Yeah worth a shot, I'm confident that it'll work when ignoring those tests which were broken on macOS from the beginning

headius · 2022-03-31T21:20:02Z

@charleskorn Sorry for the delay wrapping this up. I just pushed a commit that disables the problematic tests and will have a release out shortly.

charleskorn · 2022-03-31T21:20:56Z

No worries at all, thanks @headius.

headius · 2022-03-31T23:01:40Z

jnr-ffi 2.2.12 has been released and should propagate soon!

charleskorn added a commit to batect/docker-client that referenced this pull request Feb 21, 2022

Disable JVM tests until jnr/jnr-ffi#299 is resolved.

f33031a

headius merged commit 46661c2 into jnr:master Mar 3, 2022

headius added this to the 2.2.12 milestone Mar 3, 2022

basshelal mentioned this pull request Mar 20, 2022

Investigate failing Pointer Boolean tests on Darwin (macOS) #302

Open

ahorek mentioned this pull request Jan 21, 2023

backport Update JNR dependencies jruby/jruby#7590

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue where string references aren't always completed with a null terminator. #299

Fix issue where string references aren't always completed with a null terminator. #299

charleskorn commented Feb 19, 2022

charleskorn commented Mar 2, 2022

headius commented Mar 3, 2022

charleskorn commented Mar 3, 2022

headius commented Mar 3, 2022

headius commented Mar 3, 2022

headius commented Mar 3, 2022

charleskorn commented Mar 3, 2022

charleskorn commented Mar 3, 2022

headius commented Mar 5, 2022

charleskorn commented Mar 6, 2022

charleskorn commented Mar 8, 2022

headius commented Mar 10, 2022

headius commented Mar 10, 2022

headius commented Mar 10, 2022

charleskorn commented Mar 12, 2022

charleskorn commented Mar 20, 2022

charleskorn commented Mar 20, 2022

basshelal commented Mar 20, 2022

charleskorn commented Mar 20, 2022

basshelal commented Mar 20, 2022

headius commented Mar 22, 2022

basshelal commented Mar 22, 2022

headius commented Mar 31, 2022

charleskorn commented Mar 31, 2022

headius commented Mar 31, 2022

Fix issue where string references aren't always completed with a null terminator. #299

Fix issue where string references aren't always completed with a null terminator. #299

Conversation

charleskorn commented Feb 19, 2022

charleskorn commented Mar 2, 2022

headius commented Mar 3, 2022

charleskorn commented Mar 3, 2022

headius commented Mar 3, 2022

headius commented Mar 3, 2022

headius commented Mar 3, 2022

charleskorn commented Mar 3, 2022

charleskorn commented Mar 3, 2022

headius commented Mar 5, 2022

charleskorn commented Mar 6, 2022

charleskorn commented Mar 8, 2022

headius commented Mar 10, 2022

headius commented Mar 10, 2022

headius commented Mar 10, 2022

charleskorn commented Mar 12, 2022

charleskorn commented Mar 20, 2022

charleskorn commented Mar 20, 2022

basshelal commented Mar 20, 2022

charleskorn commented Mar 20, 2022

basshelal commented Mar 20, 2022

headius commented Mar 22, 2022

basshelal commented Mar 22, 2022

headius commented Mar 31, 2022

charleskorn commented Mar 31, 2022

headius commented Mar 31, 2022