Open chipitsine opened 4 months ago
No, we actually need to do this before goto error
we need
od_router_close(router, auth_client);
od_router_unroute(router, auth_client);
od_client_free(auth_client);
in line 276
full report is
96int od_auth_query(od_client_t *client, char *peer)
97{
98 od_global_t *global = client->global;
99 od_rule_t *rule = client->rule;
100 od_rule_storage_t *storage = rule->storage;
101 kiwi_var_t *user = &client->startup.user;
102 kiwi_password_t *password = &client->password;
103 od_instance_t *instance = global->instance;
104 od_router_t *router = global->router;
105
106 /* check odyssey storage auh query cache before
107 * doing any actual work
108 */
109 /* username -> password cache */
110 od_hashmap_elt_t *value;
111 od_hashmap_elt_t key;
112 od_auth_cache_value_t *cache_value;
113 od_hash_t keyhash;
114 uint64_t current_time;
115
116 key.data = user->value;
117 key.len = user->value_len;
118
119 keyhash = od_murmur_hash(key.data, key.len);
120 /* acquire hash map entry lock */
121 value = od_hashmap_lock_key(storage->acache, keyhash, &key);
122
1. Condition value->data == NULL, taking true branch.
123 if (value->data == NULL) {
124 /* one-time initialize */
125 value->len = sizeof(od_auth_cache_value_t);
126 value->data = malloc(value->len);
127 /* OOM */
2. Condition value->data == NULL, taking false branch.
128 if (value->data == NULL) {
129 goto error;
130 }
131 memset(((od_auth_cache_value_t *)(value->data)), 0, value->len);
132 }
133
134 cache_value = (od_auth_cache_value_t *)value->data;
135
136 current_time = machine_time_us();
137
3. Condition current_time - cache_value->timestamp < 10000000UL /* 10 * interval_usec */, taking false branch.
138 if (/* password cached for 10 sec */
139 current_time - cache_value->timestamp < 10 * interval_usec) {
140 od_debug(&instance->logger, "auth_query", NULL, NULL,
141 "reusing cached password for user %.*s",
142 user->value_len, user->value);
143 /* unlock hashmap entry */
144 password->password_len = cache_value->passwd_len;
145 if (cache_value->passwd_len > 0) {
146 /* */
147 password->password = malloc(password->password_len + 1);
148 if (password->password == NULL) {
149 goto error;
150 }
151 strncpy(password->password, cache_value->passwd,
152 cache_value->passwd_len);
153 password->password[password->password_len] = '\0';
154 }
155 od_hashmap_unlock_key(storage->acache, keyhash, &key);
156 return OK_RESPONSE;
157 }
158
159 /* create internal auth client */
160 od_client_t *auth_client;
161
162 auth_client = od_client_allocate_internal(global, "auth-query");
163
4. Condition auth_client == NULL, taking false branch.
164 if (auth_client == NULL) {
165 od_debug(&instance->logger, "auth_query", auth_client, NULL,
166 "failed to allocate internal auth query client");
167 goto error;
168 }
169
170 od_debug(&instance->logger, "auth_query", auth_client, NULL,
171 "acquiring password for user %.*s", user->value_len,
172 user->value);
173
174 /* set auth query route user and database */
175 kiwi_var_set(&auth_client->startup.user, KIWI_VAR_UNDEF,
176 rule->auth_query_user, strlen(rule->auth_query_user) + 1);
177
178 kiwi_var_set(&auth_client->startup.database, KIWI_VAR_UNDEF,
179 rule->auth_query_db, strlen(rule->auth_query_db) + 1);
180
181 /* set io from client */
182 od_io_t auth_client_io = auth_client->io;
183 auth_client->io = client->io;
184
185 /* route */
186 od_router_status_t status;
5. lock: od_router_route locks router->lock.[show details]
187 status = od_router_route(router, auth_client);
188
189 /* return io auth_client back */
190 auth_client->io = auth_client_io;
191
6. Condition status != OD_ROUTER_OK, taking true branch.
192 if (status != OD_ROUTER_OK) {
193 od_debug(&instance->logger, "auth_query", auth_client, NULL,
194 "failed to route internal auth query client: %s",
195 od_router_status_to_str(status));
196 od_client_free(auth_client);
7. Jumping to label error.
197 goto error;
198 }
199
200 /* attach */
201 status = od_router_attach(router, auth_client, false);
202 if (status != OD_ROUTER_OK) {
203 od_debug(
204 &instance->logger, "auth_query", auth_client, NULL,
205 "failed to attach internal auth query client to route: %s",
206 od_router_status_to_str(status));
207 od_router_unroute(router, auth_client);
208 od_client_free(auth_client);
209 goto error;
210 }
211 od_server_t *server;
212 server = auth_client->server;
213
214 od_debug(&instance->logger, "auth_query", auth_client, server,
215 "attached to server %s%.*s", server->id.id_prefix,
216 (int)sizeof(server->id.id), server->id.id);
217
218 /* connect to server, if necessary */
219 int rc;
220 if (server->io.io == NULL) {
221 /* acquire new backend connection for auth query */
222 rc = od_backend_connect(server, "auth_query", NULL,
223 auth_client);
224 if (rc == NOT_OK_RESPONSE) {
225 od_debug(&instance->logger, "auth_query", auth_client,
226 server,
227 "failed to acquire backend connection: %s",
228 od_io_error(&server->io));
229 od_router_close(router, auth_client);
230 od_router_unroute(router, auth_client);
231 od_client_free(auth_client);
232 goto error;
233 }
234 }
235
236 /* preformat and execute query */
237 char query[OD_QRY_MAX_SZ];
238 char *format_pos = rule->auth_query;
239 char *format_end = rule->auth_query + strlen(rule->auth_query);
240 od_query_format(format_pos, format_end, user, peer, query,
241 sizeof(query));
242
243 machine_msg_t *msg;
244 msg = od_query_do(server, "auth_query", query, user->value);
245 if (msg == NULL) {
246 od_log(&instance->logger, "auth_query", auth_client, server,
247 "auth query returned empty msg");
248 od_router_close(router, auth_client);
249 od_router_unroute(router, auth_client);
250 od_client_free(auth_client);
251 goto error;
252 }
253 if (od_auth_parse_passwd_from_datarow(&instance->logger, msg,
254 password) == NOT_OK_RESPONSE) {
255 od_debug(&instance->logger, "auth_query", auth_client, server,
256 "auth query returned datarow in incompatable format");
257 od_router_close(router, auth_client);
258 od_router_unroute(router, auth_client);
259 od_client_free(auth_client);
260 goto error;
261 }
262
263 /* save received password and recieve timestamp */
264 if (cache_value->passwd != NULL) {
265 /* drop previous value */
266 free(cache_value->passwd);
267
268 // there should be cache_value->passwd = NULL for sanity
269 // but this is meaninigless sinse we assing new value just below
270 }
271 cache_value->passwd_len = password->password_len;
272 cache_value->passwd = malloc(password->password_len);
273 if (cache_value->passwd == NULL) {
274 goto error;
275 }
276 strncpy(cache_value->passwd, password->password,
277 cache_value->passwd_len);
278
279 cache_value->timestamp = current_time;
280
281 /* detach and unroute */
282 od_router_detach(router, auth_client);
283 od_router_unroute(router, auth_client);
284 od_client_free(auth_client);
285 od_hashmap_unlock_key(storage->acache, keyhash, &key);
286 return OK_RESPONSE;
287
288error:
289 /* unlock hashmap entry */
290 od_hashmap_unlock_key(storage->acache, keyhash, &key);
CID 486482: (#1 of 1): Missing unlock (LOCK)
8. missing_unlock: Returning without unlocking router->lock.
291 return NOT_OK_RESPONSE;
292}
do you mean line 196 ? I do not see issues on 276
line 274 273 if (cache_value->passwd == NULL) { 274 goto error; 275 }
Coverity is aware of jump from line 196 to line 288. I tried to address that issue.
line 274 is outside the scope of this fix
(I think that 196 --> 274 jump is low priority because it is part of error path, we do not care much about error path, but Coverity has no idea on that unfortunately)
found by Coverity