use embedded javascript engine for pac logic and remove dependency with third party pacparser library

[fralken]

This PR removes the dependency with third party pacparser library by using the embedded javascript engine Duktape. Duktape is easy to integrate in a project by adding its source code. On top of this the logic to deal with pac files is implemented (in pac.c and pac.h). This has the following advantages:

• no third party dependency, so that it's easier to distribute the binaries in various platforms
• lower memory footprint (in my scenario I had 30-40MB with pacparser and only 4-5KB with duktape)

The downside is:

• slightly longer compilation time
• larger binary size

[versat]

That's really awesome. How about removing the conditional configuration/compilation of PAC support and simply always include the support? That would make configure/make and code way more simple and readable. Since there is now no longer any runtime dependency for PAC support, I do not see any reason to not have it in the builds. What do you think?

[fralken]

I agree with you. For now I left the conditional compilation logic because it was already in place. I was wondering if there are any use cases for which pac support is not required and a smaller, less complex runtime is needed. An alternative could be to reverse the condition, with a --disable-pac option for disabling this feature that otherwise is compiled by default.

[versat]

Reversing the condition so it is possible to disable the PAC feature is a good idea. But then the code is still more complex than it might have to be. Maybe later this condition then can be removed completely if there are no "complaints" :) For now I think it is good to go

[fralken]

Only the configure file would be impacted by reversing the parameter (in the code #ifdef ENABLE_PAC would remain. Anyway I agree that probably it would be a useless complexity. Does anybody need a version of cntlm without PAC support? @jschwartzenberg what do you think?

[jschwartzenberg]

Only the configure file would be impacted by reversing the parameter (in the code #ifdef ENABLE_PAC would remain.

I think it would be best to remove the #ifdefs also to reduce complexity. I see no reason to have a version without PAC support. I think it's a great new use case for CNTLM even when no authentication is needed.

[fralken]

I removed the conditional compilation (that is the "--enable-pac" config option). @jschwartzenberg and @versat can you do some tests and check if it's ok for you?

[jschwartzenberg]

I think this clean-up makes the code much easier to read. @versat, what do you think?

I'll try to do some testing.

[fralken]

Unfortunately the SolarCloud Code Analysis fails, but in my opinion they are false positives.

[versat]

Looks good to me so far. Sadly I can not test it much. I'm fine with not fixing all SonarQube issues in this PR, especially the ones from Duktape. Maybe we can even exclude Duktape from the analysis. Some issues, no matter if false positives or not, should be simple to fix for example by adding another null pointer check or by assigning \0 to the last character before calling strlen. But this can be done separately.

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

0.0% Coverage
0.0% Duplication

[fralken]

Yes you're right, so I excluded duktape code from SonarCloud analysis. Also, I fixed the reported critical issues (and some code smells), it wasn't that difficult and indeed the code should be more robust now.

[versat]

Sadly Coverty analysis is not possible for PRs, but we got the result for the master branch now: @fralken Maybe you can have a look :)

Here is the report:

Please find the latest report on new defect(s) introduced to versat/cntlm found with Coverity Scan.

4 new defect(s) introduced to versat/cntlm found with Coverity Scan.

New defect(s) Reported-by: Coverity Scan Showing 4 of 4 defect(s)

** CID 379866: API usage errors (USE_AFTER_FREE)

---

** CID 379866: API usage errors (USE_AFTER_FREE) /main.c: 370 in tunnel_thread() 364 pos = 0; 365
366 if (noproxy_match(hostname)) { 367 direct_tunnel(thread_data); 368 } else { 369 if (forward_tunnel(thread_data) == -2)

CID 379866:  API usage errors  (USE_AFTER_FREE)
Calling "direct_tunnel(void *)" closes handle "thread_data->fd" which has already been closed.

370 direct_tunnel(thread_data); 371 } 372
373 free(hostname); 374 free(thread_data); 375

** CID 379865: Memory - illegal accesses (STRING_NULL)

---

** CID 379865: Memory - illegal accesses (STRING_NULL) /pac.c: 160 in pac_parse_file() 154
155 int pac_parse_file(const char pacfile) { 156 char *pacstring = read_file(pacfile); 157 if (!pacstring) 158 return 0; 159

CID 379865:  Memory - illegal accesses  (STRING_NULL)
Passing unterminated string "pacstring" to "pac_parse_string", which expects a null-terminated string.

160 int rc = pac_parse_string(pacstring); 161 free(pacstring); 162
163 return rc; 164 } 165

** CID 379864: (ATOMICITY) /proxy.c: 329 in paclist_create() /proxy.c: 316 in paclist_create()

---

** CID 379864: (ATOMICITY) /proxy.c: 329 in paclist_create() 323 proxy = (proxy_t )zmalloc(sizeof(proxy_t)); 324 proxy->type = DIRECT; 325
326 pthread_mutex_lock(&parent_mtx); 327 ++parent_count; 328 parent_list = proxylist_add(parent_list, parent_count, proxy);

CID 379864:    (ATOMICITY)
Using an unreliable value of "plist" inside the second locked section. If the data that "plist" depends on was changed by another thread, this use might be incorrect.

329 plist = proxylist_add(plist, parent_count, proxy); 330 pthread_mutex_unlock(&parent_mtx); 331 } 332 } 333 if (p != NULL) 334 plist = proxylist_add(plist, p->key, p->proxy); /proxy.c: 316 in paclist_create() 310 while (p != NULL && !(p->proxy->type == type && p->proxy->port == iport && !strcmp(p->proxy->hostname, hostname))) 311 p = p->next; 312 if (p == NULL) { 313 pthread_mutex_lock(&parent_mtx); 314 parent_add(hostname, iport); 315 proxy = proxylist_get(parent_list, parent_count); CID 379864: (ATOMICITY) Using an unreliable value of "plist" inside the second locked section. If the data that "plist" depends on was changed by another thread, this use might be incorrect. 316 plist = proxylist_add(plist, parent_count, proxy); 317 pthread_mutex_unlock(&parent_mtx); 318 } 319 } else { // type == DIRECT 320 while (p != NULL && p->proxy->type != type) 321 p = p->next;

** CID 31700: Error handling issues (CHECKED_RETURN) /pac.c: 101 in read_file()

---

** CID 31700: Error handling issues (CHECKED_RETURN) /pac.c: 101 in read_file() 95 buf = (char)calloc(len+1, sizeof(char));
96 if(buf == NULL) { 97 fclose(fd); 98 return NULL; 99 } 100

CID 31700:  Error handling issues  (CHECKED_RETURN)
"fread(void *, unsigned long, unsigned long, FILE *)" returns the number of bytes read, but it is ignored.

101 fread(buf, sizeof(char), len, fd); 102 fclose(fd); 103
104 return buf; 105 } 106

---

To view the defects in Coverity Scan visit, https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50ypxt-2FJd3fQz6vEwa5utJg-2FcTnFwjX1sRDJxlmbNumejhFoe41jO6oNu1MoXRNV52x4-3DzamH_iyK5RBXdSBjxU-2Fm42g93KDIgRWLjAQUT9KfYCnbioS4MLnMaSLJMjhub2tJn2hIqttbB-2FeuD-2Btyg7fc-2FSf4do9vmj3zdoThc3Fw957HvzFMypwJwGMbPsVxl9t8e4DDYCw2R-2FTkXGoazkVQvU9iJfRrteHAKS9037YmG-2B9O0ZzdRyn-2Fc5nAtGv6ny2o76tc219n4h5r4N-2FgFBd1EyaVzbg-3D-3D

[fralken]

Oh, this is crazy. Indeed the USE_AFTER_FREE is a bug and I'm fixing it.

The STRING_NULL issue is weird because a buffer of len+1 chars is allocated and zeroed (with calloc) and then len chars are read from file. So in the end the resulting string should be null terminated.

The ATOMICITY issues are related to a local variable that depends on thread shared global variable, that's why it is modified inside a mutex.

Finally, for CHECKED_RETURN I don't really need to check the number of bytes read.

All but the first issue appear to me as false positives, but maybe I'm missing something.

[versat]

For the STRING_NULL issue you can simply write something like pacstring[LEN - 1] = '\0'; before it is used to be absolutely sure it is null terminated. For the ATOMACITY issue I agree. It looks like Coverty does not see the mutex locking. Regarding the CHECKED_RETURN issue you can simply write (void) fread(... to tell all static analyzers that it is intended to not use the return value.

[fralken]

Ok I committed the changes for STRING_NULL and CHECKED_RETURN, let's see. For ATOMICITY, maybe it complains that the same variable is manipulated in two different locked sections. But I don't know how it can be silenced.

[fralken]

I see that if I allocate a buffer buf of len+1 size and then I set buf[len] = 0 SonarCloud complains for index out of bound. This is fighting against analysis tools.

So I removed the commit. It doesn't make sense to me to affect code readability only to silence false positive issues.