Merge lp:~stefan.goetz-deactivatedaccount/hipl/msg-bfr into lp:hipl
- msg-bfr
- Merge into trunk
Status: | Merged |
---|---|
Merge reported by: | Stefan Götz |
Merged at revision: | not available |
Proposed branch: | lp:~stefan.goetz-deactivatedaccount/hipl/msg-bfr |
Merge into: | lp:hipl |
Diff against target: |
365 lines (+83/-100) 6 files modified
hipd/cookie.c (+6/-29) hipd/cookie.h (+5/-6) hipd/hidb.c (+0/-6) hipd/output.c (+53/-54) hipd/output.h (+6/-5) lib/core/protodefs.h (+13/-0) |
To merge this branch: | bzr merge lp:~stefan.goetz-deactivatedaccount/hipl/msg-bfr |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Miika Komu | Approve | ||
Christof Mroz | Approve | ||
Stefan Götz (community) | Abstain | ||
Diego Biurrun | Abstain | ||
Review via email: mp+59993@code.launchpad.net |
Commit message
Description of the change
Improve robustness in memory management: in the 'struct r1entry' data structure, convert the 'r1' field from a 'struct hip_common' pointer to a 'union hip_msg_bfr' object so it does not need to be allocated dynamically.
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : | # |
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : | # |
'hip_create_r1' no longer initializes its message object to 0 before constructing the message data. Either, the message buffer should be reset to 0 before constructing the message or it needs to be confirmed that the message construction code itself is sufficient to leave no uninitialized data in the message buffer.
As I'd like to avoid the resetting, I'll check the message construction code as I find the time.
Thanks to Christof Mroz for pointing this out.
Reviews apart from this aspect are still welcome.
- 5911. By René Hummen
-
fix debug output for received packets
Previously, src was printed instead of dst.
Diego Biurrun (diego-biurrun) wrote : | # |
review abstain
On Wed, May 04, 2011 at 09:09:02PM +0000, Stefan Götz wrote:
> Stefan Götz has proposed merging lp:~stefan.goetz/hipl/msg-bfr into lp:hipl.
>
> --- hipd/cookie.h 2011-04-19 16:22:10 +0000
> +++ hipd/cookie.h 2011-05-04 21:08:48 +0000
> @@ -32,11 +32,11 @@
>
> struct hip_r1entry {
> - struct hip_common *r1;
> - uint32_t generation;
> - uint8_t Ci[PUZZLE_LENGTH];
> - uint8_t Ck;
> - uint8_t Copaque[
> + union hip_msg_bfr bfr;
> + uint32_t generation;
> + uint8_t Ci[PUZZLE_LENGTH];
> + uint8_t Ck;
> + uint8_t Copaque[
> };
>
> --- lib/core/
> +++ lib/core/
> @@ -701,6 +701,19 @@
>
> +union hip_msg_bfr {
> + struct hip_common msg;
> + struct hip_common_user usr;
> + unsigned char bfr[HIP_
> +};
You use "bfr" as a shorthand for "buffer", but I think "buf" is way more
common. I suggest using the latter.
Diego
Miika Komu (miika-iki) wrote : | # |
The builder assumes that the message buffer has been initialized to zeroes. Uninitialized messages will crash both the network and interprocess communications.
I fail to see the reason for this non-zeroing micro optimization in this, particularly because it would require rewriting the builder and lots of testing. If the goal is to improve processing overhead, I think this micro-optimization won't help much. If it's about DoS protection, I think threading would have a larger impact.
So my recommendation is to zero the message. As an alternative, you can modify the builder and test it properly (doesn't seem worth the trouble).
Christof Mroz (christof-mroz) wrote : | # |
> I fail to see the reason for this non-zeroing micro optimization in this,
> particularly because it would require rewriting the builder and lots of
> testing. If the goal is to improve processing overhead, I think this micro-
> optimization won't help much. If it's about DoS protection, I think threading
> would have a larger impact.
Not sure about Stefan, but that's certainly not what I had in mind. The question was essentially: is the builder blown apart by not zeroing the buffer right after allocation, or will it be zeroied before (re-)building the precomputed R1s anyway (which would be the sensible thing to do)?
If that's the case, the branch can be commited without modifications IMO.
- 5912. By David Martin
-
Remove pointless debug print from hip_netdev_event().
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : | # |
I briefly looked into the builder to see whether it is robust against unzeroed message buffers. It is not :-)
> The builder assumes that the message buffer has been initialized to zeroes.
> Uninitialized messages will crash both the network and interprocess
> communications.
It crashes or, in the case of R1s, it aborts early as it might find that, given an uninitialized message length field, the message is already too large to add more parameters.
> I fail to see the reason for this non-zeroing micro optimization in this,
> particularly because it would require rewriting the builder and lots of
> testing. If the goal is to improve processing overhead, I think this micro-
> optimization won't help much.
I agree with you given the current overall processing overhead in HIPL. [Offtopic: However, it might make sense to deliberately tackle this overhead. For example, the pre-created R1 messages are duplicated before they are sent which is probably unnecessary. Also the sending routine moves the complete message contents around in memory twice when using raw sockets. Thus I believe there is quite some potential for performance improvements, especially on slow platforms such as WiFi routers. But, yes, of course, it's a lot of work not relevant for this branch.]
> So my recommendation is to zero the message. As an alternative, you can modify
> the builder and test it properly (doesn't seem worth the trouble).
Yes. I'll update the branch accordingly.
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : | # |
> You use "bfr" as a shorthand for "buffer", but I think "buf" is way more
> common. I suggest using the latter.
Done. Is there anything in particular I can do to get an 'approve' from you?
Stefan Götz (stefan.goetz-deactivatedaccount) : | # |
Stefan Götz (stefan.goetz-deactivatedaccount) : | # |
Miika Komu (miika-iki) wrote : | # |
Another small comment. This was removed from the R1 deallocation procedure:
43 - hip_r1table = NULL;
While this is strictly speaking not necessary, I have found it easier to debug NULL pointer crashes rather than pointers that are freed.
Christof Mroz (christof-mroz) wrote : | # |
If it works, I'm fine with it apart from one minor thing:
> === modified file 'hipd/output.c'
> --- hipd/output.c 2011-05-02 14:19:14 +0000
> +++ hipd/output.c 2011-05-07 06:46:29 +0000
> @@ -568,6 +568,8 @@
> /**
> * Construct a new R1 packet payload
> *
> + * @param msg points to a message object backed by HIP_MAX_PACKET bytes
> + * of memory to which the R1 message is written.
> * @param src_hit a pointer to the source host identity tag used in the
> * packet.
> * @param sign a funtion pointer to a signature funtion.
> @@ -576,19 +578,20 @@
>
> * @param cookie_k the difficulty value for the puzzle
> * @return a pointer to the payload on success, NULL on error.
> */
You modified the function to return -1 rather than NULL.
- 5913. By Stefan Götz
-
Merge branch https:/
/code.launchpad .net/~stefan. goetz/hipl/ lhi-storage
Merge proposal https://code.launchpad .net/~stefan. goetz/hipl/ lhi-storage/ +merge/ 59992 - 5917. By Stefan Götz
-
Initialize the message buffer as the message builder depends on it.
- 5918. By Stefan Götz
-
Rename the 'bfr' field in 'struct hip_r1entry' to 'buf' as this name is
more common. - 5919. By Stefan Götz
-
Fix incorrect documentation of return value of hip_create_r1()
Fixes https:/
/code.launchpad .net/~stefan. goetz/hipl/ msg-bfr/ +merge/ 59993/comments/ 130604
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : | # |
Hi Miika!
> Another small comment. This was removed from the R1 deallocation procedure:
>
> 43 - hip_r1table = NULL;
>
> While this is strictly speaking not necessary, I have found it easier to debug
> NULL pointer crashes rather than pointers that are freed.
True. Given that hip_r1table is a local variable and that it is no longer allocated dynamically, it does not apply in this case.
Unless there is anything else, would you give me an 'approve' on the merge proposal?
Stefan
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : | # |
Hi Christof!
> You modified the function to return -1 rather than NULL.
I updated the documentation accordingly.
Unless there is anything else, would you give me an 'approve' on the merge proposal?
Stefan
Christof Mroz (christof-mroz) wrote : | # |
> I updated the documentation accordingly.
>
> Unless there is anything else, would you give me an 'approve' on the merge
> proposal?
Yep, looks fine now.
Miika Komu (miika-iki) : | # |
Preview Diff
1 | === modified file 'hipd/cookie.c' |
2 | --- hipd/cookie.c 2011-05-04 16:20:00 +0000 |
3 | +++ hipd/cookie.c 2011-05-10 18:14:26 +0000 |
4 | @@ -211,9 +211,9 @@ |
5 | HIP_DEBUG("Calculated index: %d\n", idx); |
6 | |
7 | /* Create a copy of the found entry */ |
8 | - len = hip_get_msg_total_len(hip_r1table[idx].r1); |
9 | + len = hip_get_msg_total_len(&hip_r1table[idx].buf.msg); |
10 | r1 = hip_msg_alloc(); |
11 | - memcpy(r1, hip_r1table[idx].r1, len); |
12 | + memcpy(r1, &hip_r1table[idx].buf.msg, len); |
13 | err = r1; |
14 | |
15 | out_err: |
16 | @@ -245,8 +245,9 @@ |
17 | |
18 | cookie_k = hip_get_cookie_difficulty(); |
19 | |
20 | - r1table[i].r1 = hip_create_r1(hit, sign, privkey, pubkey, cookie_k); |
21 | - if (!r1table[i].r1) { |
22 | + hip_msg_init(&r1table[i].buf.msg); |
23 | + |
24 | + if (hip_create_r1(&r1table[i].buf.msg, hit, sign, privkey, pubkey, cookie_k)) { |
25 | HIP_ERROR("Unable to precreate R1s\n"); |
26 | return 0; |
27 | } |
28 | @@ -258,29 +259,6 @@ |
29 | } |
30 | |
31 | /** |
32 | - * uninitialize R1 table |
33 | - * |
34 | - * @param hip_r1table R1 table |
35 | - */ |
36 | -void hip_uninit_r1(struct hip_r1entry *const hip_r1table) |
37 | -{ |
38 | - int i; |
39 | - |
40 | - /* The R1 packet consist of 2 memory blocks. One contains the actual |
41 | - * buffer where the packet is formed, while the other contains |
42 | - * pointers to different TLVs to speed up parsing etc. |
43 | - * The r1->common is the actual buffer, and r1 is the structure |
44 | - * holding only pointers to the TLVs. |
45 | - */ |
46 | - if (hip_r1table) { |
47 | - for (i = 0; i < HIP_R1TABLESIZE; i++) { |
48 | - free(hip_r1table[i].r1); |
49 | - hip_r1table[i].r1 = NULL; |
50 | - } |
51 | - } |
52 | -} |
53 | - |
54 | -/** |
55 | * Verifies the solution of a puzzle. First we check that K and I are the same |
56 | * as in the puzzle we sent. If not, then we check the previous ones (since the |
57 | * puzzle might just have been expired). |
58 | @@ -314,7 +292,7 @@ |
59 | -1, "Requested source HIT not (any more) available.\n"); |
60 | result = &hid->r1[hip_calc_cookie_idx(ip_i, ip_r)]; |
61 | |
62 | - puzzle = hip_get_param(result->r1, HIP_PARAM_PUZZLE); |
63 | + puzzle = hip_get_param(&result->buf.msg, HIP_PARAM_PUZZLE); |
64 | HIP_IFEL(!puzzle, -1, "Internal error: could not find the cookie\n"); |
65 | HIP_IFEL(memcmp(solution->opaque, puzzle->opaque, |
66 | HIP_PUZZLE_OPAQUE_LEN), -1, |
67 | @@ -369,7 +347,6 @@ |
68 | { |
69 | int (*signature_func)(void *key, struct hip_common *m); |
70 | |
71 | - hip_uninit_r1(entry->r1); |
72 | switch (hip_get_host_id_algo(&entry->host_id)) { |
73 | case HIP_HI_RSA: |
74 | signature_func = hip_rsa_sign; |
75 | |
76 | === modified file 'hipd/cookie.h' |
77 | --- hipd/cookie.h 2011-05-04 16:10:54 +0000 |
78 | +++ hipd/cookie.h 2011-05-10 18:14:26 +0000 |
79 | @@ -34,17 +34,16 @@ |
80 | #define HIP_R1TABLESIZE 3 /* precreate only this many R1s */ |
81 | |
82 | struct hip_r1entry { |
83 | - struct hip_common *r1; |
84 | - uint32_t generation; |
85 | - uint8_t Ci[PUZZLE_LENGTH]; |
86 | - uint8_t Ck; |
87 | - uint8_t Copaque[HIP_PUZZLE_OPAQUE_LEN]; |
88 | + union hip_msg_bfr buf; |
89 | + uint32_t generation; |
90 | + uint8_t Ci[PUZZLE_LENGTH]; |
91 | + uint8_t Ck; |
92 | + uint8_t Copaque[HIP_PUZZLE_OPAQUE_LEN]; |
93 | }; |
94 | |
95 | struct hip_common *hip_get_r1(struct in6_addr *ip_i, |
96 | struct in6_addr *ip_r, |
97 | struct in6_addr *peer_hit); |
98 | -void hip_uninit_r1(struct hip_r1entry *const hip_r1table); |
99 | int hip_recreate_all_precreated_r1_packets(void); |
100 | int hip_precreate_r1(struct hip_r1entry *r1table, |
101 | const struct in6_addr *hit, |
102 | |
103 | === modified file 'hipd/hidb.c' |
104 | --- hipd/hidb.c 2011-05-04 16:20:00 +0000 |
105 | +++ hipd/hidb.c 2011-05-10 18:14:26 +0000 |
106 | @@ -245,12 +245,6 @@ |
107 | id->remove(id, &id->arg); |
108 | } |
109 | |
110 | - /* free the dynamically reserved memory and |
111 | - * set host_id to null to signal that it is free */ |
112 | - if (id->r1) { |
113 | - hip_uninit_r1(id->r1); |
114 | - } |
115 | - |
116 | switch (hip_get_host_id_algo(&id->host_id)) { |
117 | case HIP_HI_RSA: |
118 | RSA_free(id->private_key); |
119 | |
120 | === modified file 'hipd/output.c' |
121 | --- hipd/output.c 2011-05-04 16:20:00 +0000 |
122 | +++ hipd/output.c 2011-05-10 18:14:26 +0000 |
123 | @@ -568,27 +568,30 @@ |
124 | /** |
125 | * Construct a new R1 packet payload |
126 | * |
127 | + * @param msg points to a message object backed by HIP_MAX_PACKET bytes |
128 | + * of memory to which the R1 message is written. |
129 | * @param src_hit a pointer to the source host identity tag used in the |
130 | * packet. |
131 | * @param sign a funtion pointer to a signature funtion. |
132 | * @param private_key a pointer to the local host private key |
133 | * @param host_id_pub a pointer to the public host id of the local host |
134 | * @param cookie_k the difficulty value for the puzzle |
135 | - * @return a pointer to the payload on success, NULL on error. |
136 | + * @return 0 on success, a non-zero value on error. |
137 | */ |
138 | -struct hip_common *hip_create_r1(const struct in6_addr *src_hit, |
139 | - int (*sign)(void *key, struct hip_common *m), |
140 | - void *private_key, |
141 | - const struct hip_host_id *host_id_pub, |
142 | - int cookie_k) |
143 | +int hip_create_r1(struct hip_common *const msg, |
144 | + const struct in6_addr *src_hit, |
145 | + int (*sign)(void *key, struct hip_common *m), |
146 | + void *private_key, |
147 | + const struct hip_host_id *host_id_pub, |
148 | + int cookie_k) |
149 | { |
150 | - struct hip_common *err = NULL; |
151 | - struct hip_srv service_list[HIP_TOTAL_EXISTING_SERVICES]; |
152 | - uint8_t *dh_data1 = NULL, *dh_data2 = NULL; |
153 | - char order[] = "000"; |
154 | - int dh_size1 = 0, dh_size2 = 0; |
155 | - int mask = 0, i = 0, written1 = 0, written2 = 0; |
156 | - unsigned int service_count = 0; |
157 | + int err = 0; |
158 | + struct hip_srv service_list[HIP_TOTAL_EXISTING_SERVICES]; |
159 | + uint8_t *dh_data1 = NULL, *dh_data2 = NULL; |
160 | + char order[] = "000"; |
161 | + int dh_size1 = 0, dh_size2 = 0; |
162 | + int mask = 0, i = 0, written1 = 0, written2 = 0; |
163 | + unsigned int service_count = 0; |
164 | |
165 | enum number_dh_keys_t { ONE, TWO }; |
166 | enum number_dh_keys_t number_dh_keys = TWO; |
167 | @@ -628,83 +631,84 @@ |
168 | } |
169 | } |
170 | |
171 | - HIP_IFEL(!(err = hip_msg_alloc()), NULL, "Out of memory\n"); |
172 | + /* Initialize the message buffer as the message builder depends on it. */ |
173 | + hip_msg_init(msg); |
174 | |
175 | /* Allocate memory for writing the first Diffie-Hellman shared secret */ |
176 | HIP_IFEL((dh_size1 = hip_get_dh_size(HIP_FIRST_DH_GROUP_ID)) == 0, |
177 | - NULL, "Could not get dh_size1\n"); |
178 | + -1, "Could not get dh_size1\n"); |
179 | HIP_IFEL(!(dh_data1 = calloc(1, dh_size1)), |
180 | - NULL, "Failed to alloc memory for dh_data1\n"); |
181 | + -1, "Failed to alloc memory for dh_data1\n"); |
182 | |
183 | /* Allocate memory for writing the second Diffie-Hellman shared secret */ |
184 | HIP_IFEL((dh_size2 = hip_get_dh_size(HIP_SECOND_DH_GROUP_ID)) == 0, |
185 | - NULL, "Could not get dh_size2\n"); |
186 | + -1, "Could not get dh_size2\n"); |
187 | HIP_IFEL(!(dh_data2 = calloc(1, dh_size2)), |
188 | - NULL, "Failed to alloc memory for dh_data2\n"); |
189 | + -1, "Failed to alloc memory for dh_data2\n"); |
190 | |
191 | /* Ready to begin building of the R1 packet */ |
192 | |
193 | /** @todo TH: hip_build_network_hdr has to be replaced with an |
194 | * appropriate function pointer */ |
195 | HIP_DEBUG_HIT("src_hit used to build r1 network header", src_hit); |
196 | - hip_build_network_hdr(err, HIP_R1, mask, src_hit, NULL); |
197 | + hip_build_network_hdr(msg, HIP_R1, mask, src_hit, NULL); |
198 | |
199 | /********** R1_COUNTER (OPTIONAL) *********/ |
200 | |
201 | /********** PUZZLE ************/ |
202 | const uint8_t zero_i[PUZZLE_LENGTH] = { 0 }; |
203 | |
204 | - HIP_IFEL(hip_build_param_puzzle(err, cookie_k, |
205 | - 42 /* 2^(42-32) sec lifetime */, 0, zero_i), |
206 | - NULL, "Cookies were burned. Bummer!\n"); |
207 | + HIP_IFEL((err = hip_build_param_puzzle(msg, cookie_k, |
208 | + 42 /* 2^(42-32) sec lifetime */, 0, zero_i)), |
209 | + err, "Cookies were burned. Bummer!\n"); |
210 | |
211 | /* Parameter Diffie-Hellman */ |
212 | HIP_IFEL((written1 = hip_insert_dh(dh_data1, dh_size1, |
213 | HIP_FIRST_DH_GROUP_ID)) < 0, |
214 | - NULL, "Could not extract the first DH public key\n"); |
215 | + written1, "Could not extract the first DH public key\n"); |
216 | |
217 | if (number_dh_keys == TWO) { |
218 | HIP_IFEL((written2 = hip_insert_dh(dh_data2, dh_size2, |
219 | HIP_SECOND_DH_GROUP_ID)) < 0, |
220 | - NULL, "Could not extract the second DH public key\n"); |
221 | + written2, "Could not extract the second DH public key\n"); |
222 | |
223 | - HIP_IFEL(hip_build_param_diffie_hellman_contents(err, |
224 | - HIP_FIRST_DH_GROUP_ID, dh_data1, written1, |
225 | - HIP_SECOND_DH_GROUP_ID, dh_data2, written2), |
226 | - NULL, "Building of DH failed.\n"); |
227 | + HIP_IFEL((err = hip_build_param_diffie_hellman_contents(msg, |
228 | + HIP_FIRST_DH_GROUP_ID, dh_data1, written1, |
229 | + HIP_SECOND_DH_GROUP_ID, dh_data2, written2)), |
230 | + err, "Building of DH failed.\n"); |
231 | } else { |
232 | - HIP_IFEL(hip_build_param_diffie_hellman_contents(err, |
233 | - HIP_FIRST_DH_GROUP_ID, dh_data1, written1, |
234 | - HIP_MAX_DH_GROUP_ID, dh_data2, 0), |
235 | - NULL, "Building of DH failed.\n"); |
236 | + HIP_IFEL((err = hip_build_param_diffie_hellman_contents(msg, |
237 | + HIP_FIRST_DH_GROUP_ID, dh_data1, written1, |
238 | + HIP_MAX_DH_GROUP_ID, dh_data2, 0)), |
239 | + err, "Building of DH failed.\n"); |
240 | } |
241 | |
242 | /* Parameter HIP transform. */ |
243 | - HIP_IFEL(hip_build_param_hip_transform(err, |
244 | - transform_hip_suite, |
245 | - sizeof(transform_hip_suite) / |
246 | - sizeof(hip_transform_suite)), |
247 | - NULL, "Building of HIP transform failed\n"); |
248 | + HIP_IFEL((err = hip_build_param_hip_transform(msg, |
249 | + transform_hip_suite, |
250 | + sizeof(transform_hip_suite) / |
251 | + sizeof(hip_transform_suite))), |
252 | + err, "Building of HIP transform failed\n"); |
253 | |
254 | /* Parameter HOST_ID */ |
255 | - HIP_IFEL(hip_build_param_host_id(err, host_id_pub), |
256 | - NULL, "Building of host id failed\n"); |
257 | + HIP_IFEL((err = hip_build_param_host_id(msg, host_id_pub)), |
258 | + err, "Building of host id failed\n"); |
259 | |
260 | /* Parameter REG_INFO */ |
261 | hip_get_active_services(service_list, &service_count); |
262 | HIP_DEBUG("Found %d active service(s) \n", service_count); |
263 | - hip_build_param_reg_info(err, service_list, service_count); |
264 | + hip_build_param_reg_info(msg, service_list, service_count); |
265 | |
266 | /* Parameter ESP-ENC transform. */ |
267 | - HIP_IFEL(hip_build_param_esp_transform(err, |
268 | - transform_esp_suite, |
269 | - sizeof(transform_esp_suite) / |
270 | - sizeof(hip_transform_suite)), |
271 | - NULL, "Building of ESP transform failed\n"); |
272 | + HIP_IFEL((err = hip_build_param_esp_transform(msg, |
273 | + transform_esp_suite, |
274 | + sizeof(transform_esp_suite) / |
275 | + sizeof(hip_transform_suite))), |
276 | + err, "Building of ESP transform failed\n"); |
277 | |
278 | /********** ESP-PROT transform (OPTIONAL) **********/ |
279 | |
280 | - HIP_IFEL(esp_prot_r1_add_transforms(err), NULL, |
281 | + HIP_IFEL((err = esp_prot_r1_add_transforms(msg)), err, |
282 | "failed to add optional esp transform parameter\n"); |
283 | |
284 | /********** ECHO_REQUEST_SIGN (OPTIONAL) *********/ |
285 | @@ -713,7 +717,7 @@ |
286 | |
287 | /* Parameter Signature 2 */ |
288 | |
289 | - HIP_IFEL(sign(private_key, err), NULL, "Signing of R1 failed.\n"); |
290 | + HIP_IFEL((err = sign(private_key, msg)), err, "Signing of R1 failed.\n"); |
291 | |
292 | /* Parameter ECHO_REQUEST (OPTIONAL) */ |
293 | |
294 | @@ -721,7 +725,7 @@ |
295 | { |
296 | struct hip_puzzle *pz; |
297 | |
298 | - HIP_IFEL(!(pz = hip_get_param_readwrite(err, HIP_PARAM_PUZZLE)), NULL, |
299 | + HIP_IFEL(!(pz = hip_get_param_readwrite(msg, HIP_PARAM_PUZZLE)), -1, |
300 | "Internal error\n"); |
301 | |
302 | /* hardcode kludge */ |
303 | @@ -733,15 +737,10 @@ |
304 | |
305 | /* Packet ready */ |
306 | |
307 | - free(dh_data1); |
308 | - free(dh_data2); |
309 | - |
310 | - return err; |
311 | - |
312 | out_err: |
313 | - free(err); |
314 | free(dh_data1); |
315 | free(dh_data2); |
316 | + |
317 | return err; |
318 | } |
319 | |
320 | |
321 | === modified file 'hipd/output.h' |
322 | --- hipd/output.h 2011-04-11 12:56:12 +0000 |
323 | +++ hipd/output.h 2011-05-10 18:14:26 +0000 |
324 | @@ -45,11 +45,12 @@ |
325 | extern int hip_raw_sock_v4; |
326 | |
327 | |
328 | -struct hip_common *hip_create_r1(const struct in6_addr *src_hit, |
329 | - int (*sign)(void *key, struct hip_common *m), |
330 | - void *private_key, |
331 | - const struct hip_host_id *host_id_pub, |
332 | - int cookie_k); |
333 | +int hip_create_r1(struct hip_common *const msg, |
334 | + const struct in6_addr *src_hit, |
335 | + int (*sign)(void *key, struct hip_common *m), |
336 | + void *private_key, |
337 | + const struct hip_host_id *host_id_pub, |
338 | + int cookie_k); |
339 | |
340 | int hip_send_r1(const uint8_t packet_type, |
341 | const uint32_t ha_state, |
342 | |
343 | === modified file 'lib/core/protodefs.h' |
344 | --- lib/core/protodefs.h 2011-05-02 14:17:17 +0000 |
345 | +++ lib/core/protodefs.h 2011-05-10 18:14:26 +0000 |
346 | @@ -701,6 +701,19 @@ |
347 | } __attribute__ ((packed)); |
348 | |
349 | /** |
350 | + * A memory buffer for a HIP message. |
351 | + * Instances of this type occupy HIP_MAX_PACKET bytes of memory. |
352 | + * This type is useful where HIP messages are constructed and to express that |
353 | + * the message needs to be backed by memory for that. |
354 | + * It is also useful where dynamic allocation of message buffers is unwanted. |
355 | + */ |
356 | +union hip_msg_bfr { |
357 | + struct hip_common msg; |
358 | + struct hip_common_user usr; |
359 | + unsigned char bfr[HIP_MAX_PACKET]; |
360 | +}; |
361 | + |
362 | +/** |
363 | * Use accessor functions defined in hip_build.h, do not access members |
364 | * directly to avoid hassle with byte ordering and length conversion. |
365 | */ |
Hi Christoph!
> Review: Approve
> Getting rid of pointers is always nice, go ahead please.
Thanks!
>> -struct hip_r1entry *hip_init_r1(void) HIP_R1TABLESIZE , sizeof(struct hip_r1entry)))) {
>> -{
>> - struct hip_r1entry *err;
>> -
>> - if (!(err = calloc(
>> - return NULL;
>> - }
>> -
>> - return err;
>> -}
>
> The memory was reset to zero by calloc() before, but currently it isn't. Are you sure there's no way uninitialized data can leak onto the wire? As padding, for example.
> OTOH, padding should really be cleared wherever the R1 is constructed, if needed (thus this comment is unrelated to your code).
This question is relevant for the 'r1' field in 'struct hip_r1entry' as all
other fields are assigned during initialization. With the 'r1' field, indeed the
R1 construction takes care of clearing the memory before creating the R1
message. However, the msg-bfr merge proposal turns that field from a pointer
into the struct instance and by dropping the memory allocation also drops the
initialization to 0. That needs to be looked into.
Stefan