From 7db3b623bd115c096f4a261d441698b20dc39d7c Mon Sep 17 00:00:00 2001
From: Rob Swindell <rob@synchro.net>
Date: Tue, 13 Apr 2021 22:57:16 -0700
Subject: [PATCH] Refactor putuserrec()
This started with a Coverity issue (CID 33230) which got me looking at this function and wondering: why is str2 being NUL-terminated here? Why is the length of str2 to be calculated on successive lines? What is with this (long)((long)((long)((long)) typecast?
This was some of the oldest code in Synchronet (along with a lot of the other functions in this file). I tried to keep as much intact as possible while still improving the logic and readability.
---
src/sbbs3/userdat.c | 81 ++++++++++++++++++++++++---------------------
src/sbbs3/userdat.h | 2 +-
2 files changed, 45 insertions(+), 38 deletions(-)
diff --git a/src/sbbs3/userdat.c b/src/sbbs3/userdat.c
index ada3b2ded3..be8f0c90f1 100644
--- a/src/sbbs3/userdat.c
+++ b/src/sbbs3/userdat.c
@@ -2217,66 +2217,73 @@ int getuserrec(scfg_t* cfg, int usernumber,int start, int length, char *str)
}
/****************************************************************************/
-/* Places into user.dat at the offset for usernumber+start for length bytes */
-/* Called from various locations */
+/* Write a string (str) into user.dat at the offset for usernumber + start */
+/* 'length' may be auto-determined (from 'start') by passing 0 for length */
+/* If 'str' is longer than 'length', only 'length' characters are written */
/****************************************************************************/
-int putuserrec(scfg_t* cfg, int usernumber,int start, uint length, const char *str)
+int putuserrec(scfg_t* cfg, int usernumber, int start, int length, const char *str)
{
- char str2[256];
+ const char* p;
+ char buf[256];
+ char path[MAX_PATH + 1];
int file;
- uint c,i;
+ uint i;
- if(!VALID_CFG(cfg) || usernumber<1 || str==NULL)
- return(-1);
+ if(!VALID_CFG(cfg) || usernumber < 1 || str == NULL)
+ return -1;
- if(length > sizeof(str2) - 1)
- return -10;
+ if(length == 0) { /* auto-length */
+ length = user_rec_len(start);
+ if(length < 1)
+ return -2;
+ }
+ size_t slen = strlen(str);
+ if(slen >= (size_t)length)
+ p = str;
+ else {
+ if(length > sizeof(buf))
+ return -10;
+ memset(buf, ETX, length);
+ memcpy(buf, str, slen);
+ p = buf;
+ }
- SAFEPRINTF(str2,"%suser/user.dat",cfg->data_dir);
- if((file=nopen(str2,O_RDWR|O_DENYNONE))==-1)
- return(errno);
+ SAFEPRINTF(path, "%suser/user.dat", cfg->data_dir);
+ if((file = nopen(path, O_RDWR|O_DENYNONE))==-1)
+ return errno;
- if(filelength(file)<((long)usernumber-1)*U_LEN) {
+ off_t offset = (usernumber - 1) * U_LEN;
+ if(filelength(file) < offset) {
close(file);
- return(-4);
- }
-
- if(length==0) { /* auto-length */
- length=user_rec_len(start);
- if((long)length < 0) {
- close(file);
- return -2;
- }
+ return -4;
}
- SAFECOPY(str2,str);
- if(strlen(str2)<length) {
- for(c=strlen(str2);c<length;c++)
- str2[c]=ETX;
- str2[c]=0;
+ offset += start;
+ if(lseek(file, offset, SEEK_SET) != offset) {
+ close(file);
+ return -5;
}
- lseek(file,(long)((long)((long)((long)usernumber-1)*U_LEN)+start),SEEK_SET);
i=0;
- while(i<LOOP_NODEDAB
- && lock(file,(long)((long)(usernumber-1)*U_LEN)+start,length)==-1) {
+ while(i < LOOP_NODEDAB
+ && lock(file, offset, length) == -1) {
if(i)
mswait(100);
i++;
}
- if(i>=LOOP_NODEDAB) {
+ if(i >= LOOP_NODEDAB) {
close(file);
- return(-3);
+ return -3;
}
- int wr = write(file, str2, length);
- unlock(file,(long)((long)(usernumber-1)*U_LEN)+start,length);
+ int wr = write(file, p, length);
+ unlock(file, offset, length);
close(file);
if(wr != length)
- return -4;
- dirtyuserdat(cfg,usernumber);
- return(0);
+ return -6;
+ dirtyuserdat(cfg, usernumber);
+ return 0;
}
/****************************************************************************/
diff --git a/src/sbbs3/userdat.h b/src/sbbs3/userdat.h
index 2f47bfadbd..9dc1d7915f 100644
--- a/src/sbbs3/userdat.h
+++ b/src/sbbs3/userdat.h
@@ -81,7 +81,7 @@ DLLEXPORT uint userdatdupe(scfg_t*, uint usernumber, uint offset, uint datlen, c
DLLEXPORT BOOL chk_ar(scfg_t*, uchar* str, user_t*, client_t*); /* checks access requirements */
DLLEXPORT int getuserrec(scfg_t*, int usernumber, int start, int length, char *str);
-DLLEXPORT int putuserrec(scfg_t*, int usernumber, int start, uint length, const char *str);
+DLLEXPORT int putuserrec(scfg_t*, int usernumber, int start, int length, const char *str);
DLLEXPORT ulong adjustuserrec(scfg_t*, int usernumber, int start, int length, long adj);
DLLEXPORT BOOL logoutuserdat(scfg_t*, user_t*, time_t now, time_t logontime);
DLLEXPORT void resetdailyuserdat(scfg_t*, user_t*, BOOL write);
--
GitLab