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

[charleskorn]

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.

[charleskorn]

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

[headius]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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

[headius]

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]

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]

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]

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

[charleskorn]

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 43aa31eddb5b458643e44c57215aab6e76d399d6 / #293, and they fail for me if I check out that commit - ie. they've been broken since they were introduced.

[charleskorn]

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]

@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]

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]

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

Thanks for this information though

[headius]

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

[basshelal]

@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]

@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]

No worries at all, thanks @headius.

[headius]

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