Skip to content

Commit 6a8ebd8

Browse files
acvacv
committed
Refactor http_get and https_get to not reuse memory
Previous implementation "magically" knew caller's buffer size and made assumption. New implementation, allocates as needed. goto usage is *not* an anti-pattern here. It's basically a C form of try { ..... } finally { ..... }
1 parent c1ba649 commit 6a8ebd8

File tree

4 files changed

+106
-81
lines changed

4 files changed

+106
-81
lines changed

‎src/centralserver.c‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ auth_server_request(t_authresponse *authresponse, const char *request_type, cons
105105

106106
free(safe_token);
107107

108-
int res = 0;
108+
char *res;
109109
#ifdef USE_CYASSL
110110
if (auth_server->authserv_use_ssl) {
111111
res = https_get(sockfd, buf, auth_server->authserv_hostname);
@@ -116,22 +116,23 @@ auth_server_request(t_authresponse *authresponse, const char *request_type, cons
116116
#ifndef USE_CYASSL
117117
res = http_get(sockfd, buf);
118118
#endif
119-
if (res < 0) {
119+
if (NULL == res) {
120120
debug(LOG_ERR, "There was a problem talking to the auth server!");
121121
return (AUTH_ERROR);
122122
}
123-
124123

125-
if ((tmp = strstr(buf, "Auth: "))) {
124+
if ((tmp = strstr(res, "Auth: "))) {
126125
if (sscanf(tmp, "Auth: %d", (int *)&authresponse->authcode) == 1) {
127126
debug(LOG_INFO, "Auth server returned authentication code %d", authresponse->authcode);
127+
free(res);
128128
return(authresponse->authcode);
129129
} else {
130130
debug(LOG_WARNING, "Auth server did not return expected authentication code");
131+
free(res);
131132
return(AUTH_ERROR);
132133
}
133134
}
134-
135+
free(res);
135136
return(AUTH_ERROR);
136137
}
137138

‎src/ping_thread.c‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ ping(void)
173173
VERSION,
174174
auth_server->authserv_hostname);
175175

176-
int res = 0;
176+
char *res;
177177
#ifdef USE_CYASSL
178178
if (auth_server->authserv_use_ssl) {
179179
res = https_get(sockfd, request, auth_server->authserv_hostname);
@@ -184,24 +184,26 @@ ping(void)
184184
#ifndef USE_CYASSL
185185
res = http_get(sockfd, request);
186186
#endif
187-
if (res < 0) {
187+
if (NULL == res) {
188188
debug(LOG_ERR, "There was a problem pinging the auth server!");
189-
}
190-
191-
if (strstr(request, "Pong") == 0) {
189+
if (!authdown) {
190+
fw_set_authdown();
191+
authdown = 1;
192+
}
193+
} else if (strstr(res, "Pong") == 0) {
192194
debug(LOG_WARNING, "Auth server did NOT say Pong!");
193195
if (!authdown) {
194196
fw_set_authdown();
195197
authdown = 1;
196198
}
197-
}
198-
else {
199+
free(res);
200+
} else {
199201
debug(LOG_DEBUG, "Auth Server Says: Pong");
200202
if (authdown) {
201203
fw_set_authup();
202204
authdown = 0;
203205
}
206+
free(res);
204207
}
205-
206208
return;
207209
}

‎src/simple_http.c‎

Lines changed: 81 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
/* vim: set sw=4 ts=4 sts=4 et : */
12
/********************************************************************\
23
* This program is free software; you can redistribute it and/or *
34
* modify it under the terms of the GNU General Public License as *
@@ -34,6 +35,7 @@
3435
#include "../config.h"
3536
#include "common.h"
3637
#include "debug.h"
38+
#include "pstring.h"
3739

3840
#ifdef USE_CYASSL
3941
#include <cyassl/ssl.h>
@@ -48,31 +50,35 @@
4850
static CYASSL_CTX *get_cyassl_ctx(void);
4951
#endif
5052

51-
52-
int
53-
http_get(const int sockfd, char *buf) {
54-
53+
/**
54+
* Perform an HTTP request, caller frees both request and response,
55+
* NULL returned on error.
56+
* @param sockfd Socket to use, already connected
57+
* @param req Request to send, fully formatted.
58+
* @return char Response as a string
59+
*/
60+
char *
61+
http_get(const int sockfd, const char *req)
62+
{
5563
ssize_t numbytes;
56-
size_t totalbytes;
5764
int done, nfds;
5865
fd_set readfds;
5966
struct timeval timeout;
60-
size_t buflen = strlen(buf);
67+
size_t reqlen = strlen(req);
68+
char readbuf[MAX_BUF];
69+
char *retval;
70+
pstr_t *response = pstr_new();
6171

6272
if (sockfd == -1) {
6373
/* Could not connect to server */
6474
debug(LOG_ERR, "Could not open socket to server!");
65-
return -1;
75+
goto error;
6676
}
6777

68-
debug(LOG_DEBUG, "Sending HTTP request to auth server: [%s]\n", buf);
69-
send(sockfd, buf, buflen, 0);
70-
71-
// Clear buffer and re-use for response
72-
memset(buf, 0, buflen);
78+
debug(LOG_DEBUG, "Sending HTTP request to auth server: [%s]\n", req);
79+
send(sockfd, req, reqlen, 0);
7380

7481
debug(LOG_DEBUG, "Reading response");
75-
totalbytes = 0;
7682
done = 0;
7783
do {
7884
FD_ZERO(&readfds);
@@ -86,40 +92,40 @@ http_get(const int sockfd, char *buf) {
8692
if (nfds > 0) {
8793
/** We don't have to use FD_ISSET() because there
8894
* was only one fd. */
89-
numbytes = read(sockfd, buf + totalbytes, MAX_BUF - (totalbytes + 1));
95+
memset(readbuf, 0, MAX_BUF);
96+
numbytes = read(sockfd, readbuf, MAX_BUF - 1);
9097
if (numbytes < 0) {
9198
debug(LOG_ERR, "An error occurred while reading from server: %s", strerror(errno));
92-
/* FIXME */
93-
close(sockfd);
94-
return -1;
99+
goto error;
95100
}
96101
else if (numbytes == 0) {
97102
done = 1;
98103
}
99104
else {
100-
totalbytes += (size_t) numbytes;
101-
debug(LOG_DEBUG, "Read %d bytes, total now %d", numbytes, totalbytes);
105+
pstr_cat(response, readbuf);
106+
debug(LOG_DEBUG, "Read %d bytes", numbytes);
102107
}
103108
}
104109
else if (nfds == 0) {
105110
debug(LOG_ERR, "Timed out reading data via select() from auth server");
106-
/* FIXME */
107-
close(sockfd);
108-
return -1;
111+
goto error;
109112
}
110113
else if (nfds < 0) {
111114
debug(LOG_ERR, "Error reading data via select() from auth server: %s", strerror(errno));
112-
/* FIXME */
113-
close(sockfd);
114-
return -1;
115+
goto error;
115116
}
116117
} while (!done);
117118

118119
close(sockfd);
119-
120-
buf[totalbytes] = '\0';
121-
debug(LOG_DEBUG, "HTTP Response from Server: [%s]", buf);
122-
return totalbytes;
120+
retval = pstr_to_string(response);
121+
debug(LOG_DEBUG, "HTTP Response from Server: [%s]", retval);
122+
return retval;
123+
124+
error:
125+
close(sockfd);
126+
retval = pstr_to_string(response);
127+
free(retval);
128+
return NULL;
123129
}
124130

125131

@@ -198,38 +204,49 @@ get_cyassl_ctx(void)
198204
}
199205

200206

201-
int
202-
https_get(const int sockfd, char *buf, const char* hostname) {
203-
207+
/**
208+
* Perform an HTTPS request, caller frees both request and response,
209+
* NULL returned on error.
210+
* @param sockfd Socket to use, already connected
211+
* @param req Request to send, fully formatted.
212+
* @param hostname Hostname to use in https request. Caller frees.
213+
* @return char Response as a string
214+
*/
215+
char *
216+
https_get(const int sockfd, const char *req, const char* hostname)
217+
{
204218
ssize_t numbytes;
205-
size_t totalbytes;
206219
int done, nfds;
207220
fd_set readfds;
208221
struct timeval timeout;
209222
unsigned long sslerr;
210223
char sslerrmsg[CYASSL_MAX_ERROR_SZ];
211-
size_t buflen = strlen(buf);
224+
size_t reqlen = strlen(req);
225+
char readbuf[MAX_BUF];
226+
char *retval;
227+
pstr_t *response = pstr_new();
228+
CYASSL *ssl = NULL;
229+
CYASSL_CTX *ctx = NULL;
212230

213231
s_config *config;
214232
config = config_get_config();
215233

216-
CYASSL_CTX* ctx = get_cyassl_ctx();
234+
ctx = get_cyassl_ctx();
217235
if (NULL == ctx) {
218236
debug(LOG_ERR, "Could not get CyaSSL Context!");
219-
return -1;
237+
goto error;
220238
}
221239

222240
if (sockfd == -1) {
223241
/* Could not connect to server */
224242
debug(LOG_ERR, "Could not open socket to server!");
225-
return -1;
243+
goto error;
226244
}
227245

228246
/* Create CYASSL object */
229-
CYASSL* ssl;
230247
if( (ssl = CyaSSL_new(ctx)) == NULL) {
231248
debug(LOG_ERR, "Could not create CyaSSL context.");
232-
return -1;
249+
goto error;
233250
}
234251
if (config->ssl_verify) {
235252
// Turn on domain name check
@@ -240,25 +257,21 @@ https_get(const int sockfd, char *buf, const char* hostname) {
240257
CyaSSL_set_fd(ssl, sockfd);
241258

242259

243-
debug(LOG_DEBUG, "Sending HTTPS request to auth server: [%s]\n", buf);
244-
numbytes = CyaSSL_send(ssl, buf, (int) buflen, 0);
260+
debug(LOG_DEBUG, "Sending HTTPS request to auth server: [%s]\n", req);
261+
numbytes = CyaSSL_send(ssl, req, (int) reqlen, 0);
245262
if (numbytes <= 0) {
246263
sslerr = (unsigned long) CyaSSL_get_error(ssl, numbytes);
247264
CyaSSL_ERR_error_string(sslerr, sslerrmsg);
248265
debug(LOG_ERR, "CyaSSL_send failed: %s", sslerrmsg);
249-
return -1;
266+
goto error;
250267
}
251-
else if (numbytes != (int) buflen) {
268+
else if ((size_t)numbytes != reqlen) {
252269
debug(LOG_ERR, "CyaSSL_send failed: only %d bytes out of %d bytes sent!",
253-
numbytes, buflen);
254-
return -1;
270+
numbytes, reqlen);
271+
goto error;
255272
}
256273

257-
// Clear buffer and re-use for response
258-
memset(buf, 0, buflen);
259-
260274
debug(LOG_DEBUG, "Reading response");
261-
totalbytes = 0;
262275
done = 0;
263276
do {
264277
FD_ZERO(&readfds);
@@ -272,47 +285,50 @@ https_get(const int sockfd, char *buf, const char* hostname) {
272285
if (nfds > 0) {
273286
/** We don't have to use FD_ISSET() because there
274287
* was only one fd. */
275-
numbytes = CyaSSL_read(ssl, buf + totalbytes, MAX_BUF - (totalbytes + 1));
288+
memset(readbuf, 0, MAX_BUF);
289+
numbytes = CyaSSL_read(ssl, readbuf, MAX_BUF - 1);
276290
if (numbytes < 0) {
277291
sslerr = (unsigned long) CyaSSL_get_error(ssl, numbytes);
278292
CyaSSL_ERR_error_string(sslerr, sslerrmsg);
279293
debug(LOG_ERR, "An error occurred while reading from server: %s", sslerrmsg);
280-
/* FIXME */
281-
close(sockfd);
282-
return -1;
294+
goto error;
283295
}
284296
else if (numbytes == 0) {
285297
/* CyaSSL_read returns 0 on a clean shutdown or if the peer closed the
286298
connection. We can't distinguish between these cases right now. */
287299
done = 1;
288300
}
289301
else {
290-
totalbytes += (size_t) numbytes;
291-
debug(LOG_DEBUG, "Read %d bytes, total now %d", numbytes, totalbytes);
302+
pstr_cat(response, readbuf);
303+
debug(LOG_DEBUG, "Read %d bytes", numbytes);
292304
}
293305
}
294306
else if (nfds == 0) {
295307
debug(LOG_ERR, "Timed out reading data via select() from auth server");
296-
/* FIXME */
297-
close(sockfd);
298-
return -1;
308+
goto error;
299309
}
300310
else if (nfds < 0) {
301311
debug(LOG_ERR, "Error reading data via select() from auth server: %s", strerror(errno));
302-
/* FIXME */
303-
close(sockfd);
304-
return -1;
312+
goto error;
305313
}
306314
} while (!done);
307315

308316
close(sockfd);
309317

310-
buf[totalbytes] = '\0';
311-
debug(LOG_DEBUG, "HTTP Response from Server: [%s]", buf);
312-
313318
CyaSSL_free(ssl);
314319

315-
return totalbytes;
320+
retval = pstr_to_string(response);
321+
debug(LOG_DEBUG, "HTTPS Response from Server: [%s]", retval);
322+
return retval;
323+
324+
error:
325+
if (ssl) {
326+
CyaSSL_free(ssl);
327+
}
328+
close(sockfd);
329+
retval = pstr_to_string(response);
330+
free(retval);
331+
return NULL;
316332
}
317333

318334

‎src/simple_http.h‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
/* vim: set sw=4 ts=4 sts=4 et : */
12
/********************************************************************\
23
* This program is free software; you can redistribute it and/or *
34
* modify it under the terms of the GNU General Public License as *
@@ -18,8 +19,13 @@
1819
* *
1920
\********************************************************************/
2021

21-
int http_get(const int, char*);
22+
#ifndef _SIMPLE_HTTP_H_
23+
#define _SIMPLE_HTTP_H_
24+
25+
char *http_get(const int, const char *);
2226

2327
#ifdef USE_CYASSL
24-
int https_get(const int, char*, const char*);
25-
#endif
28+
char *https_get(const int, const char *, const char *);
29+
#endif /* defined(USE_CYASSL) */
30+
31+
#endif /* defined(_SIMPLE_HTTP_H_) */

0 commit comments

Comments
 (0)