kezhuw commented on PR #2209: URL: https://github.com/apache/zookeeper/pull/2209#issuecomment-2502369147
1. I have pointed that "there is no guarantee that zcert.ca will always point to allocated memory after strtok" since https://github.com/apache/zookeeper/pull/2209#issuecomment-2458627959. (and repeated also in https://github.com/apache/zookeeper/pull/2209#issuecomment-2466111427 https://github.com/apache/zookeeper/pull/2209#issuecomment-2484562704). Given cert str **,,,/tmp/certs/server.crt,/tmp/certs/client.crt,/tmp/certs/clientkey.pem,password**(be aware of leading **,,,**), `cert.certstr` and `cert.ca` will point to different memory address. 2. Is this a brand new user-facing API ? -1 if it is the case. I don't think people want and will use a separate API to validate the cert str. All changes should accommodate to existing code path. 3. -1 on this. This is the current state of this pr. I have pointed out the problem of this approach. https://github.com/apache/zookeeper/pull/2209#issuecomment-2458627959 https://github.com/apache/zookeeper/pull/2209#issuecomment-2466111427 https://github.com/apache/zookeeper/pull/2209#issuecomment-2484562704. > I believe that (2) is the right approach, but willing to accept suggestions. I suggest you to focus on fixing the memory leaking problem and leave changes to API behavior to new issue. Personally, I have no willing to change existing behavior of API. > should certstr be removed from cert_t structure since certstr is not holding the whole string If you are worried about misuse of `cert.certstr`, it would be nice to comment it a bit. Say: ```diff typedef struct _zcert { + // Points to memory region from `malloc`, don't use it for any purposes other than `free`. char *certstr; ``` Combining with fix in https://github.com/apache/zookeeper/pull/2209#issuecomment-2458627959, the result is sufficient for us to ship it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@zookeeper.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
