Hi Marcel,
On 07/04/2011 09:47 PM, Marcel Holtmann wrote:
Hi Jukka,
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
+struct cache_entry {
+ GHashTable *cache;
why do you have a cache hast table inside the cache entry?
Hmm, it seems not to be needed, I will remove it.
+static struct cache_entry *cache_check(struct server_data *server,
+ gpointer request)
+{
+ uint16_t type;
+ unsigned int offset = 0;
+ unsigned char *dns_body = (unsigned char *)request + 12;
+ unsigned char *question =&dns_body[offset];
+ struct cache_entry *entry;
+
+ offset = strlen((char *)question) + 1;
+ type = ntohs(*(uint16_t *)(&dns_body[offset]));
Are we sure that we do not need unaligned access here?
Good point. I did not see any problems with Atom but there might be
alignment issues in other processors.
+static int parse_response(unsigned char *buf, int buflen,
+ unsigned char *question, int qlen,
+ int *type, int *class, int *ttl,
+ unsigned char *response, unsigned int *resp_len,
+ int *answers)
+{
+ struct domain_hdr *hdr = (void *) buf;
+ unsigned char *ptr;
+ uint16_t qdcount = ntohs(hdr->qdcount);
+ uint16_t ancount = ntohs(hdr->ancount);
+ int err, i, qtype, qclass;
+ unsigned char *next = NULL;
+
+ if (buflen< 12)
+ return -EINVAL;
+
+ DBG("qr %d qdcount %d", hdr->qr, qdcount);
+
+ /* We currently only cache responses where question count is 1 */
+ if (hdr->qr != 1 || qdcount != 1)
+ return -EINVAL;
+
+ ptr = buf + sizeof(struct domain_hdr);
+
+ strncpy((char *)question, (char *)ptr, qlen);
+ qlen = strlen((char *)question);
+ ptr += qlen + 1;
+ qtype = ntohs(*(uint16_t *)ptr);
+
+ /* We cache only A and AAAA records */
+ if (qtype != 1&& qtype != 28)
+ return -ENOMSG;
+
+ ptr += 2;
+ qclass = ntohs(*(uint16_t *)ptr);
+ ptr += 2; /* ptr points now to answers */
+
+ err = -ENOMSG;
+ *resp_len = 0;
+ *answers = 0;
+
+ for (i = 0; i< ancount; i++) {
+ unsigned char rsp[128];
+ unsigned int rsp_len = sizeof(rsp) - 1;
+ int len = 0;
+
+ memset(rsp, 0, sizeof(rsp));
+ err = parse_rr(buf, ptr, buf + buflen, rsp,&rsp_len,&len,
+ type, class, ttl,&next);
+ if (err != 0)
+ return err;
+
+ if ((strncmp((char *)question, (char *)rsp, len) == 0)&&
+ *type == qtype&& *class == qclass) {
Should we also check that the len of question and rsp is well in the
limits here.
Yep, I will fix that.
+ rsplen = sizeof(response) - 1;
+ err = parse_response(msg + offset, msg_len - offset,
+ question, sizeof(question) - 1,
+ &type,&class,&ttl,
+ response,&rsplen,&answers);
+ if (err< 0)
+ return 0;
+
+ if (g_hash_table_lookup(data->cache, question) == NULL) {
Why not just != NULL and just leave the function then?
Sure, why not :)
+ int qlen = strlen((char *)question);
+ unsigned char *ptr;
+
+ entry = g_try_new0(struct cache_entry, 1);
When using g_try_new0 you need to check the result and abort nicely.
Yes, I will add that.
+ entry->cache = data->cache;
+ entry->key = (unsigned char *)g_strdup((char *)question);
+ entry->type = type;
+ entry->answers = answers;
+ entry->data_len = 12 + qlen + 1 + 2 + 2 + rsplen;
+ entry->data = ptr = g_malloc0(entry->data_len);
Here we really wanna check the result and abort nicely.
True, will do that.
+
+ memcpy(ptr, msg, 12);
+ ptr += 12;
+ memcpy(ptr, question, qlen);
+ ptr += qlen + 1;
+ *(uint16_t *)ptr = htons(type);
+ ptr += 2;
+ *(uint16_t *)ptr = htons(class);
+ ptr += 2;
+ memcpy(ptr, response, rsplen);
If you copy data in the allocated buffer anyway, then setting it to zero
first is waste.
I skipped the \0 at the end of the question (ptr += qlen + 1) because
the buffer was set to 0. I can remove the zeroing, that is fine, I just
need to remember to add the \0 in the buffer.
And no need to keep moving the pointer forward. Just use memcpy(ptr + ..
style operation.
Sure.
Also what about unaligned access. Do we need to care?
Probably it makes sense to try to play safe here, I will change that.
+ /*
+ * Restrict the cached dns entry ttl to some sane value
+ * in order to prevent data staying in the cache too long.
+ */
+ if (ttl> MAX_CACHE_TTL)
+ ttl = MAX_CACHE_TTL;
+
+ entry->timeout = g_timeout_add_seconds(ttl,
+ cache_entry_timeout,
+ entry);
+ g_hash_table_insert(data->cache, g_strdup((char *)entry->key),
+ entry);
The double allocation on entry->key is not a good idea.
Ok.
+ data->cache = g_hash_table_new_full(g_str_hash,
+ g_str_equal,
+ g_free,
+ cache_element_destroy);
I prefer to not double allocated the keys. Just have the key point to
cache->key and free it via cache_element_destroy callback.
Sure, np.
However you need to use g_hash_table_replace instead of
g_hash_table_insert to make this work properly. Otherwise you have
invalid memory access.
Ok.
Regards
Marcel
Jukka
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
