View Issue Details

IDProjectCategoryView StatusLast Update
0007625GNUnetset servicepublic2023-02-08 13:13
Reporterulfvonbelow Assigned ToChristian Grothoff  
PrioritynormalSeveritytrivialReproducibilityalways
Status assignedResolutionopen 
Product VersionGit master 
Summary0007625: Operation->inquiries_sent is full of meaningless values
DescriptionThe only place anything is inserted into this multihashmap is in decode_and_send and handle_union_pdp_inquiry in gnunet-service-setu.c. What is inserted is the address of a stack-allocated variable that promptly goes out of scope.
Steps To ReproduceRead gnunet-service-setu.c. Be confused.
Additional InformationNot an actual bug, since these values are never actually used. Still, confusing, and could lead to bugs in the future. Patch attached to stuff the enum value in question into a (void *) instead of storing a meaningless address.
Tagspatch
Attached Files
0001-SETU-put-a-sensible-value-in-inquiries_sent.patch (1,998 bytes)   
From 17afe7b3d95a4befc769f79c271a4a3361e2b067 Mon Sep 17 00:00:00 2001
From: ulfvonbelow <strilen@tilde.club>
Date: Sun, 29 Jan 2023 05:39:18 -0600
Subject: [PATCH] SETU: put a sensible value in inquiries_sent.

Currently this stuffs the address of an integer-sized stack-allocated variable
into inquiries_sent, which will be irrelevant as soon as the stack frame is
destroyed. It's a pain to allocate an integer-sized region of memory just to
hold a value that's no larger than a pointer, so for now let's use intptr_t to
cast the value. Also, none of these values are currently used.
---
 src/setu/gnunet-service-setu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/setu/gnunet-service-setu.c b/src/setu/gnunet-service-setu.c
index ba32af577..ad81dd28d 100644
--- a/src/setu/gnunet-service-setu.c
+++ b/src/setu/gnunet-service-setu.c
@@ -37,6 +37,7 @@
 #include <gcrypt.h>
 #include "gnunet_setu_service.h"
 #include "setu.h"
+#include <stdint.h>
 
 #define LOG(kind, ...) GNUNET_log_from (kind, "setu", __VA_ARGS__)
 
@@ -2917,7 +2918,7 @@ decode_and_send (struct Operation *op)
                                          hashed_key);
       GNUNET_CONTAINER_multihashmap_put (op->inquiries_sent,
                                          hashed_key,
-                                         &mcfs,
+                                         (void *) (intptr_t) mcfs,
                                          GNUNET_CONTAINER_MULTIHASHMAPOPTION_REPLACE
                                          );
 
@@ -3642,7 +3643,7 @@ handle_union_p2p_inquiry (void *cls,
                                      hashed_key);
   GNUNET_CONTAINER_multihashmap_put (op->inquiries_sent,
                                      hashed_key,
-                                     &mcfs,
+                                     (void *) (intptr_t) mcfs,
                                      GNUNET_CONTAINER_MULTIHASHMAPOPTION_REPLACE
                                      );
 
-- 
2.38.1

Activities

Christian Grothoff

2023-02-08 13:13

manager   ~0019773

Well, I think the value is actually OK, as the intent was to simply track of the keys. So any non-NULL pointer is fine. But sure, maybe something more sensible could be put in there. I'm more concerned that the map is never *used* (no get/contains). Seems like some logic from the design was omitted in the implementation.

Issue History

Date Modified Username Field Change
2023-01-29 21:18 ulfvonbelow New Issue
2023-01-29 21:18 ulfvonbelow Status new => assigned
2023-01-29 21:18 ulfvonbelow Assigned To => Florian Dold
2023-01-29 21:18 ulfvonbelow Tag Attached: patch
2023-01-29 21:18 ulfvonbelow File Added: 0001-SETU-put-a-sensible-value-in-inquiries_sent.patch
2023-02-08 10:29 schanzen Assigned To Florian Dold => Christian Grothoff
2023-02-08 13:13 Christian Grothoff Note Added: 0019773