sources/auth_query.c: improve cleanup on error path

[chipitsine]

found by Coverity

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;

[reshke]

No, we actually need to do this before goto error

[reshke]

Also od_client_free

[reshke]

we need

od_router_close(router, auth_client);
od_router_unroute(router, auth_client);
od_client_free(auth_client);

in line 276

[reshke]

If client is not routed we cannot close it, no backend connection is assigned
we also cannot unroute non-routed client

[chipitsine]

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

[reshke]

line 274
273 if (cache_value->passwd == NULL) {
274 goto error;
275 }

[chipitsine]

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)