For strncat, to obtain n, one must not subtract the length of what is appended, but the length of what is already written from the buffer size.
Verified with this little test program:
#include <stdio.h> #include <string.h>
int main() { char buf[20]; strncpy(buf, "123", 10); strncat(buf, "456789012345", 10 - strlen(buf)); printf("%s\n", buf); return 0; }
It prints "1234567890". --- gtp/gtp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gtp/gtp.c b/gtp/gtp.c index 12cb492..55a8ce4 100644 --- a/gtp/gtp.c +++ b/gtp/gtp.c @@ -650,7 +650,7 @@ static void log_restart(struct gsn_t *gsn)
filename[NAMESIZE - 1] = 0; /* No null term. guarantee by strncpy */ strncpy(filename, gsn->statedir, NAMESIZE - 1); - strncat(filename, RESTART_FILE, NAMESIZE - 1 - sizeof(RESTART_FILE)); + strncat(filename, RESTART_FILE, NAMESIZE - 1 - strlen(filename));
i = umask(022);
Put restart file in dir/gsn_restart instead of ../dirgsn_restart. --- gtp/gtp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gtp/gtp.c b/gtp/gtp.c index 55a8ce4..cdff238 100644 --- a/gtp/gtp.c +++ b/gtp/gtp.c @@ -650,7 +650,7 @@ static void log_restart(struct gsn_t *gsn)
filename[NAMESIZE - 1] = 0; /* No null term. guarantee by strncpy */ strncpy(filename, gsn->statedir, NAMESIZE - 1); - strncat(filename, RESTART_FILE, NAMESIZE - 1 - strlen(filename)); + strncat(filename, "/" RESTART_FILE, NAMESIZE - 1 - strlen(filename));
i = umask(022);
Rather nul-terminate string buffer after writing bytes. This is only cosmetic since 'NAMESIZE - 1' is passed to the strn* functions. This ordering shows the intention more clearly.
Also put the comment on its own line and explain in more detail. --- gtp/gtp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/gtp/gtp.c b/gtp/gtp.c index cdff238..c974b7b 100644 --- a/gtp/gtp.c +++ b/gtp/gtp.c @@ -648,9 +648,10 @@ static void log_restart(struct gsn_t *gsn) int counter = 0; char filename[NAMESIZE];
- filename[NAMESIZE - 1] = 0; /* No null term. guarantee by strncpy */ strncpy(filename, gsn->statedir, NAMESIZE - 1); strncat(filename, "/" RESTART_FILE, NAMESIZE - 1 - strlen(filename)); + /* guarantee nul term, strncpy might omit if too long */ + filename[NAMESIZE - 1] = 0;
i = umask(022);
This is a patch series for OpenGGSN. It was submitted before on August 18th, but this new version separates the nul termination thing to a separate commit (and slightly tweaks it).
~Neels
On Thu, Sep 15, 2016 at 03:06:38PM +0200, Neels Hofmeyr wrote:
For strncat, to obtain n, one must not subtract the length of what is appended, but the length of what is already written from the buffer size.
[...]
On 15 Sep 2016, at 15:12, Neels Hofmeyr nhofmeyr@sysmocom.de wrote:
This is a patch series for OpenGGSN. It was submitted before on August 18th, but this new version separates the nul termination thing to a separate commit (and slightly tweaks it).
I think I had raised it before. The code is not performance critical on start so why not use talloc_strdup here?
holger
On Thu, Sep 15, 2016 at 03:37:35PM +0200, Holger Freyther wrote:
I think I had raised it before. The code is not performance critical on start so why not use talloc_strdup here?
I must have missed that somehow. Makes more sense indeed.
~Neels
osmocom-net-gprs@lists.osmocom.org