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 NodeService::setIdentity(..). #175

Closed
wants to merge 1 commit into from

Conversation

aarlt
Copy link

@aarlt aarlt commented Mar 13, 2022

I think there was a typo here.

@joseph-henry
Copy link
Contributor

I don't think this is a typo. Is something not working for you? We perform this check so that one doesn't accidentally write too much data into the internal string buffer.

@aarlt
Copy link
Author

aarlt commented Mar 15, 2022

@joseph-henry I was not able to use this function. I created a new key pair with the lib, and the code always returned an error.

if (keypair == NULL || len < ZT_IDENTITY_STRING_BUFFER_LENGTH) {
   return ZTS_ERR_ARG;
}

I understood it like this:

  • if the pointer to key pair points to nullptr, return an error, or
  • if the length of the key is less than provided by the buffer.

But why should the second part be an error? Isn't it only an error, if the provided length is larger than the max. buffer size available? I think it's ok, if the length was smaller or equal than the maximal buffer length.

@aarlt
Copy link
Author

aarlt commented Mar 15, 2022

I don't think this is a typo. Is something not working for you? We perform this check so that one doesn't accidentally write too much data into the internal string buffer.

But yeah! I was also really wondering, because that would mean that nobody ever used this function so far. But maybe I really missed something here.

@joseph-henry
Copy link
Contributor

Heh. Checking test/selftest.c I have testing for zts_init_from_memory marked for "tomorrow" like 8 months ago. So you might be right about this. I'll test locally.

@someara someara deleted the branch zerotier:master January 6, 2023 14:38
@someara someara closed this Jan 6, 2023
@aarlt
Copy link
Author

aarlt commented Feb 24, 2024

Heh. Checking test/selftest.c I have testing for zts_init_from_memory marked for "tomorrow" like 8 months ago. So you might be right about this. I'll test locally.

looks like nothing changed in test/selftest.c since many years.. it's somehow a bit scary that you only uncommented the return of the error - in June 2023...

@aarlt
Copy link
Author

aarlt commented Feb 24, 2024

I hope that you simply just don't release your real tests.

joseph-henry added a commit that referenced this pull request Feb 27, 2024
@joseph-henry
Copy link
Contributor

Sorry not sure why this got closed and never properly fixed. And also, yes it looks like that was commented out to get around an issue while developing Pylon but erroneously got included in a commit.

I've now included a check so that all buffers must be exactly 384 bytes in length and added some tests in selftest.c to verify this is checked.

Thanks for pointing this out and sorry it too so long to fix.

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.

3 participants