@matheusbsbs / matheus-bagliano / commits / 390c6cbc5e

http: use strbufs instead of fixed buffers

We keep the names of incoming packs and objects in fixed PATH_MAX-size buffers, and snprintf() into them. This is unlikely to end up with truncated filenames, but it is possible (especially on systems where PATH_MAX is shorter than actual paths can be). Let's switch to using strbufs, which makes the question go away entirely. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed May 18, 2018 at 18:56 UTC 390c6cbc5e643b6d89869b319b51b5b62a3f5a09

2 files changed +38 -32

http.c

+36 -30

@@ -2087,6 +2087,7 @@ void release_http_pack_request(struct http_pack_request *preq)

2087 preq->packfile = NULL;

2088 }

2089 preq->slot = NULL;

2090 + strbuf_release(&preq->tmpfile);

2091 free(preq->url);

2092 free(preq);

2093 }

@@ -2109,19 +2110,19 @@ int finish_http_pack_request(struct http_pack_request *preq)

2110 lst = &((*lst)->next);

2111 *lst = (*lst)->next;

2112

2112 - if (!strip_suffix(preq->tmpfile, ".pack.temp", &len))

2113 + if (!strip_suffix(preq->tmpfile.buf, ".pack.temp", &len))

2114 die("BUG: pack tmpfile does not end in .pack.temp?");

2114 - tmp_idx = xstrfmt("%.*s.idx.temp", (int)len, preq->tmpfile);

2115 + tmp_idx = xstrfmt("%.*s.idx.temp", (int)len, preq->tmpfile.buf);

2116

2117 argv_array_push(&ip.args, "index-pack");

2118 argv_array_pushl(&ip.args, "-o", tmp_idx, NULL);

2118 - argv_array_push(&ip.args, preq->tmpfile);

2119 + argv_array_push(&ip.args, preq->tmpfile.buf);

2120 ip.git_cmd = 1;

2121 ip.no_stdin = 1;

2122 ip.no_stdout = 1;

2123

2124 if (run_command(&ip)) {

2124 - unlink(preq->tmpfile);

2125 + unlink(preq->tmpfile.buf);

2126 unlink(tmp_idx);

2127 free(tmp_idx);

2128 return -1;

@@ -2129,7 +2130,7 @@ int finish_http_pack_request(struct http_pack_request *preq)

2130

2131 unlink(sha1_pack_index_name(p->sha1));

2132

2132 - if (finalize_object_file(preq->tmpfile, sha1_pack_name(p->sha1))

2133 + if (finalize_object_file(preq->tmpfile.buf, sha1_pack_name(p->sha1))

2134 || finalize_object_file(tmp_idx, sha1_pack_index_name(p->sha1))) {

2135 free(tmp_idx);

2136 return -1;

@@ -2148,6 +2149,7 @@ struct http_pack_request *new_http_pack_request(

2149 struct http_pack_request *preq;

2150

2151 preq = xcalloc(1, sizeof(*preq));

2152 + strbuf_init(&preq->tmpfile, 0);

2153 preq->target = target;

2154

2155 end_url_with_slash(&buf, base_url);

@@ -2155,12 +2157,11 @@ struct http_pack_request *new_http_pack_request(

2157 sha1_to_hex(target->sha1));

2158 preq->url = strbuf_detach(&buf, NULL);

2159

2158 - snprintf(preq->tmpfile, sizeof(preq->tmpfile), "%s.temp",

2159 - sha1_pack_name(target->sha1));

2160 - preq->packfile = fopen(preq->tmpfile, "a");

2160 + strbuf_addf(&preq->tmpfile, "%s.temp", sha1_pack_name(target->sha1));

2161 + preq->packfile = fopen(preq->tmpfile.buf, "a");

2162 if (!preq->packfile) {

2163 error("Unable to open local file %s for pack",

2163 - preq->tmpfile);

2164 + preq->tmpfile.buf);

2165 goto abort;

2166 }

2167

@@ -2187,6 +2188,7 @@ struct http_pack_request *new_http_pack_request(

2188 return preq;

2189

2190 abort:

2191 + strbuf_release(&preq->tmpfile);

2192 free(preq->url);

2193 free(preq);

2194 return NULL;

@@ -2237,7 +2239,7 @@ struct http_object_request *new_http_object_request(const char *base_url,

2239 {

2240 char *hex = sha1_to_hex(sha1);

2241 struct strbuf filename = STRBUF_INIT;

2240 - char prevfile[PATH_MAX];

2242 + struct strbuf prevfile = STRBUF_INIT;

2243 int prevlocal;

2244 char prev_buf[PREV_BUF_SIZE];

2245 ssize_t prev_read = 0;

@@ -2245,40 +2247,41 @@ struct http_object_request *new_http_object_request(const char *base_url,

2247 struct http_object_request *freq;

2248

2249 freq = xcalloc(1, sizeof(*freq));

2250 + strbuf_init(&freq->tmpfile, 0);

2251 hashcpy(freq->sha1, sha1);

2252 freq->localfile = -1;

2253

2254 sha1_file_name(&filename, sha1);

2252 - snprintf(freq->tmpfile, sizeof(freq->tmpfile),

2253 - "%s.temp", filename.buf);

2255 + strbuf_addf(&freq->tmpfile, "%s.temp", filename.buf);

2256

2255 - snprintf(prevfile, sizeof(prevfile), "%s.prev", filename.buf);

2256 - unlink_or_warn(prevfile);

2257 - rename(freq->tmpfile, prevfile);

2258 - unlink_or_warn(freq->tmpfile);

2257 + strbuf_addf(&prevfile, "%s.prev", filename.buf);

2258 + unlink_or_warn(prevfile.buf);

2259 + rename(freq->tmpfile.buf, prevfile.buf);

2260 + unlink_or_warn(freq->tmpfile.buf);

2261 strbuf_release(&filename);

2262

2263 if (freq->localfile != -1)

2264 error("fd leakage in start: %d", freq->localfile);

2263 - freq->localfile = open(freq->tmpfile,

2265 + freq->localfile = open(freq->tmpfile.buf,

2266 O_WRONLY | O_CREAT | O_EXCL, 0666);

2267 /*

2268 * This could have failed due to the "lazy directory creation";

2269 * try to mkdir the last path component.

2270 */

2271 if (freq->localfile < 0 && errno == ENOENT) {

2270 - char *dir = strrchr(freq->tmpfile, '/');

2272 + char *dir = strrchr(freq->tmpfile.buf, '/');

2273 if (dir) {

2274 *dir = 0;

2273 - mkdir(freq->tmpfile, 0777);

2275 + mkdir(freq->tmpfile.buf, 0777);

2276 *dir = '/';

2277 }

2276 - freq->localfile = open(freq->tmpfile,

2278 + freq->localfile = open(freq->tmpfile.buf,

2279 O_WRONLY | O_CREAT | O_EXCL, 0666);

2280 }

2281

2282 if (freq->localfile < 0) {

2281 - error_errno("Couldn't create temporary file %s", freq->tmpfile);

2283 + error_errno("Couldn't create temporary file %s",

2284 + freq->tmpfile.buf);

2285 goto abort;

2286 }

2287

@@ -2292,7 +2295,7 @@ struct http_object_request *new_http_object_request(const char *base_url,

2295 * If a previous temp file is present, process what was already

2296 * fetched.

2297 */

2295 - prevlocal = open(prevfile, O_RDONLY);

2298 + prevlocal = open(prevfile.buf, O_RDONLY);

2299 if (prevlocal != -1) {

2300 do {

2301 prev_read = xread(prevlocal, prev_buf, PREV_BUF_SIZE);

@@ -2309,7 +2312,8 @@ struct http_object_request *new_http_object_request(const char *base_url,

2312 } while (prev_read > 0);

2313 close(prevlocal);

2314 }

2312 - unlink_or_warn(prevfile);

2315 + unlink_or_warn(prevfile.buf);

2316 + strbuf_release(&prevfile);

2317

2318 /*

2319 * Reset inflate/SHA1 if there was an error reading the previous temp

@@ -2324,7 +2328,7 @@ struct http_object_request *new_http_object_request(const char *base_url,

2328 lseek(freq->localfile, 0, SEEK_SET);

2329 if (ftruncate(freq->localfile, 0) < 0) {

2330 error_errno("Couldn't truncate temporary file %s",

2327 - freq->tmpfile);

2331 + freq->tmpfile.buf);

2332 goto abort;

2333 }

2334 }

@@ -2354,6 +2358,7 @@ struct http_object_request *new_http_object_request(const char *base_url,

2358 return freq;

2359

2360 abort:

2361 + strbuf_release(&prevfile);

2362 free(freq->url);

2363 free(freq);

2364 return NULL;

@@ -2381,25 +2386,25 @@ int finish_http_object_request(struct http_object_request *freq)

2386 if (freq->http_code == 416) {

2387 warning("requested range invalid; we may already have all the data.");

2388 } else if (freq->curl_result != CURLE_OK) {

2384 - if (stat(freq->tmpfile, &st) == 0)

2389 + if (stat(freq->tmpfile.buf, &st) == 0)

2390 if (st.st_size == 0)

2386 - unlink_or_warn(freq->tmpfile);

2391 + unlink_or_warn(freq->tmpfile.buf);

2392 return -1;

2393 }

2394

2395 git_inflate_end(&freq->stream);

2396 git_SHA1_Final(freq->real_sha1, &freq->c);

2397 if (freq->zret != Z_STREAM_END) {

2393 - unlink_or_warn(freq->tmpfile);

2398 + unlink_or_warn(freq->tmpfile.buf);

2399 return -1;

2400 }

2401 if (hashcmp(freq->sha1, freq->real_sha1)) {

2397 - unlink_or_warn(freq->tmpfile);

2402 + unlink_or_warn(freq->tmpfile.buf);

2403 return -1;

2404 }

2405

2406 sha1_file_name(&filename, freq->sha1);

2402 - freq->rename = finalize_object_file(freq->tmpfile, filename.buf);

2407 + freq->rename = finalize_object_file(freq->tmpfile.buf, filename.buf);

2408 strbuf_release(&filename);

2409

2410 return freq->rename;

@@ -2407,7 +2412,7 @@ int finish_http_object_request(struct http_object_request *freq)

2412

2413 void abort_http_object_request(struct http_object_request *freq)

2414 {

2410 - unlink_or_warn(freq->tmpfile);

2415 + unlink_or_warn(freq->tmpfile.buf);

2416

2417 release_http_object_request(freq);

2418 }

@@ -2427,4 +2432,5 @@ void release_http_object_request(struct http_object_request *freq)

2432 release_active_slot(freq->slot);

2433 freq->slot = NULL;

2434 }

2435 + strbuf_release(&freq->tmpfile);

2436 }

http.h

+2 -2

@@ -200,7 +200,7 @@ struct http_pack_request {

200 struct packed_git *target;

201 struct packed_git **lst;

202 FILE *packfile;

203 - char tmpfile[PATH_MAX];

203 + struct strbuf tmpfile;

204 struct active_request_slot *slot;

205 };

206

@@ -212,7 +212,7 @@ extern void release_http_pack_request(struct http_pack_request *preq);

212 /* Helpers for fetching object */

213 struct http_object_request {

214 char *url;

215 - char tmpfile[PATH_MAX];

215 + struct strbuf tmpfile;

216 int localfile;

217 CURLcode curl_result;

218 char errorstr[CURL_ERROR_SIZE];