empiredan commented on code in PR #2223:
URL: https://github.com/apache/zookeeper/pull/2223#discussion_r1997890812
##########
zookeeper-client/zookeeper-client-c/src/zk_sasl.c:
##########
@@ -451,41 +453,87 @@ struct zsasl_secret_ctx {
static int _zsasl_getsecret(sasl_conn_t *conn, void *context, int id,
sasl_secret_t **psecret)
{
+ const size_t MAX_PASSWORD_LEN = 1023;
struct zsasl_secret_ctx *secret_ctx = (struct zsasl_secret_ctx *)context;
- char buf[1024];
- char *password;
- size_t len;
+ char buf[MAX_PASSWORD_LEN + 1];
+ char *password = NULL;
+ size_t len = 0;
+ int res = 0;
+ char new_passwd[MAX_PASSWORD_LEN + 1];
+ char *p = NULL;
sasl_secret_t *x;
/* paranoia check */
- if (!conn || !psecret || id != SASL_CB_PASS)
+ if (!conn || !psecret || id != SASL_CB_PASS) {
return SASL_BADPARAM;
+ }
if (secret_ctx->password_file) {
- char *p;
FILE *fh = fopen(secret_ctx->password_file, "rt");
- if (!fh)
+ if (!fh) {
return SASL_FAIL;
+ }
- if (!fgets(buf, sizeof(buf), fh)) {
+ /*
+ * The file's content may be the encrypted password with binary
characters,
+ * thus use fread().
+ */
+ len = fread(buf, sizeof(buf[0]), MAX_PASSWORD_LEN, fh);
+ if (ferror(fh)) {
fclose(fh);
return SASL_FAIL;
}
fclose(fh);
+ fh = NULL;
Review Comment:
Indeed, as you mentioned, since the `fh` pointer will no longer be used
later, there is no need to assign it to `null`. The reason for handling it this
way is to prevent [**dangling
pointer**](https://en.wikipedia.org/wiki/Dangling_pointer) issues; for example,
we have similar handling in other parts of our code:
https://github.com/apache/zookeeper/blame/master/zookeeper-client/zookeeper-client-c/src/zk_sasl.c#L541.
--
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
