| Message ID | 20170928155446.32382-1-steffan@karger.me |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
Hi, On 28/09/17 23:54, Steffan Karger wrote: > If gc == NULL, the data allocated in the alloc_gc_buf() call would never > be free'd. The function is never called that way, but let's prevent > future problems. Same problem exists with the invocation of gen_path() a few lines below: it invokes string_mod_const() that invokes string_alloc(.., gc). Would you fix that as well? Under the same umbrella patch maybe? > > Signed-off-by: Steffan Karger <steffan@karger.me> > --- > src/openvpn/misc.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c > index 90632706..86f80a6a 100644 > --- a/src/openvpn/misc.c > +++ b/src/openvpn/misc.c > @@ -724,10 +724,13 @@ const char * > create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc) > { > static unsigned int counter; > - struct buffer fname = alloc_buf_gc(256, gc); > int fd; > const char *retfname = NULL; > unsigned int attempts = 0; > + struct buffer fname = (struct buffer) { 0 }; > + char buf_data[256] = { 0 }; > + > + buf_set_write(&fname, buf_data, sizeof(buf_data)); This buffer is really only used to create a filename and then it's passed to gen_path(). Is it really worth creating a struct buffer, embedding a string and then extrapolating the string again to call gen_path()? How about directly using the string directly? The only argument against this approach I can see is that buf_printf() does all the checks for us, while if we switch to a snprintf() we need to add a one-line-check to ensure the resulting string could fit fname. What do you think? I am just trying to simplify the code while at it .. Cheers,
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c index 90632706..86f80a6a 100644 --- a/src/openvpn/misc.c +++ b/src/openvpn/misc.c @@ -724,10 +724,13 @@ const char * create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc) { static unsigned int counter; - struct buffer fname = alloc_buf_gc(256, gc); int fd; const char *retfname = NULL; unsigned int attempts = 0; + struct buffer fname = (struct buffer) { 0 }; + char buf_data[256] = { 0 }; + + buf_set_write(&fname, buf_data, sizeof(buf_data)); do {
If gc == NULL, the data allocated in the alloc_gc_buf() call would never be free'd. The function is never called that way, but let's prevent future problems. Signed-off-by: Steffan Karger <steffan@karger.me> --- src/openvpn/misc.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)