[pve-devel] [PATCH v2 cluster 1/2] pmxcfs: add verify_token IPCC request

[Thomas Lamprecht]

On 11/25/19 8:02 AM, Fabian Grünbichler wrote:
>>> +
>>> +				while (line != NULL) {
>>> +					next_line = memchr(line, '\n', remaining);
>>> +					if (next_line != NULL) {
>>> +						*next_line = '\0';
>> unnecessary, just use (next_line or end) - line as boundaries and do a
>> strncmp below[0]
> but I actually want to ensure I compare both token and the full line, no 
> matter which one is longer.
> 
> e.g., if the file contains (for whatever reason!)
> someuser at pve!token asd213
> someotheruser at pve!token asdas-asdas-dsadsa
> 
> and I compare against
> 
> someuser at pve!token asd213-dfsdasads-asdsdas-dasds
> 
> I don't want to compare across the newline! while it should never be 
> able to produce a false match (we reject token values as needle that 
> contain a newline), it seems like a good safeguard to ensure we only 
> match one full line at a time..
> 
>>> +					}
>>> +					/* we ensure above that both are properly terminated */
>> you ain't if next_line was NULL, i.e., no \n at EOF, at least if you
>> do not do your out-of-bound write to *end = 0 anymore ;-)
>>
>>> +					if (strcmp(line, rh->token) == 0) {
>> [0] ...here
>>
>> effectively I'd go with:
>>
>> int bytes_read = memdb_read(memdb, "priv/token.cfg", &tmp);
>> size_t remaining = bytes_read > 0 ? bytes_read : 0;
>> if (tmp != NULL && remaining >= tokenlen) {
>>     char *line = (char *) tmp;
>>     char *next_newline;
>>     char *const end = line + remaining;
>>
>>     while (line != NULL) {
>>         next_newline = memchr(line, '\n', remaining);
>>         if (next_newline != NULL) {
>>             *next_newline = '\0';
> so in practice I'd add another check here that the current line 
> (next_newline - line) is as long as rh->token, and if it is not, proceed 
> directly to the next line. this might also speed it up, since we now 
> only do the full string/memory comparison against stored values where 
> the full tokenid has the same length as the one we are looking for.
> 
> if next_newline == NULL, then we must be in the last loop, and remaining 
> should be exactly tokenlen, so we could check that in an else branch and 
> bail if that check fails.
> 

sounds OK.

>>         }
>>
>>         if (memcmp(line, rh->token, tokenlen) == 0) {
>>             result = 0; // found
>>             break;
>>         }
>>
>>         line = next_newline;
>>         if (line != NULL) {
>>             line += 1;
>>             remaining = end - line;
>>             if (remaining < tokenlen) {
>>                 result = -ENOENT;
>>                 break;
>>             }
>>         }
>>     }
>>     if (line == NULL) {
>>         result = -ENOENT;
>>     }
>>     g_free(tmp);
>> } else {
>>     cfs_debug("token: token.cfg does not exist - ENOENT");
>>     result = -ENOENT;
>> }
>>
>> here, the actually search part could be moved out to a is_line_in_conf
>> function, as it's completely agnostic to tokens itself..
> sounds like a good idea - but where to put it? actually create the msg 
> and put most of the code in status.c, like for the property getter? or 
> keep the token stuff inline, and just move the line-search to 
> cfs-utils.c?
> 

I'd rather use cfs-utils.c or a completely new file, e.g., ipc-utils.c,
where alsot the config property getter stuff could be moved to.
If unsure just keep it inline for now.