-
-
Notifications
You must be signed in to change notification settings - Fork 197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use MessagePack for the Unix socket API #75
Conversation
Note that due to its complexity, this PR will most likely be a work in progress for quite some time and will not be part of the next versions of |
…ment removes the fixed buffer we used before and allows for responses having arbitraty lengths
…to decide whether the output whould be in telnet format, or JSON (possibly even "v2" JSON)
Currently using the cJSON library, as it's entirely contained within one source and one header file and is very easy to use. By adding on the API output formats to the previous raw socket code, we can simply change the output format when the response type is `API` instead of `SOCKET`. As shown in the implemented summary API code, this should be very simple for most commands.
…r into process_api_request() (request.c) much like we already do it for socket requests
…and http://pi.hole/admin/api.php?overTimeData10mins give identical results
request.c
Outdated
if(sscanf(client_message, ">%*[^(](%i)", &num) > 0) | ||
// SOCKET: >top-domains (15) | ||
// API: /stats/top_domains?limit=15 | ||
if(sscanf(client_message, "%*[^0123456789H\n]%i", &num) > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this pick up /stats/top_domains?alkjsdlkfj=15
as valid?
request.c
Outdated
{ | ||
desc = true; | ||
} | ||
else if(type != SOCKET && command(client_message, "desc")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should check for order=desc
request.c
Outdated
// Match both top-domains and top-ads | ||
// SOCKET: >top-clients (15) | ||
// API: /stats/top_clients?limit=15 | ||
if(sscanf(client_message, "%*[^0123456789H\n]%i", &num) > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment.
Also make query types output more like the other responses.
…the whole header was analyzed), 2. Anaylze passed GET arguments properly, e.g. allow ?limit=123, but ignore ?sfqw=123
Signed-off-by: Mcat12 <newtoncat12@yahoo.com> # Conflicts: # main.c # request.c # routines.h # socket.c
Removed reference to the HTTP API. Signed-off-by: Mcat12 <newtoncat12@yahoo.com>
Signed-off-by: Mcat12 <newtoncat12@yahoo.com>
Might have been lost in a merge. Signed-off-by: Mcat12 <newtoncat12@yahoo.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review comments for all files except api.c
and request.c
(need to use a better tool for looking at the diff
, GitHub web doesn't seem to understand that the file got renamed).
FTL.h
Outdated
@@ -23,6 +23,7 @@ | |||
#include <time.h> | |||
#include <sys/time.h> | |||
#include <sys/socket.h> | |||
#include <regex.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still use any subroutine from the regex.h header?
FTL.h
Outdated
@@ -207,7 +211,8 @@ typedef struct { | |||
enum { DATABASE_WRITE_TIMER, EXIT_TIMER }; | |||
|
|||
enum { QUERIES, FORWARDED, CLIENTS, DOMAINS, OVERTIME, WILDCARD }; | |||
enum { SOCKET }; | |||
enum { TELNET, SOCKET }; | |||
enum { WHITELIST, BLACKLIST, WILDLIST }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not be needed as well.
FTL.h
Outdated
@@ -252,3 +257,5 @@ long int lastdbindex; | |||
bool travis; | |||
bool DBdeleteoldqueries; | |||
bool rereadgravity; | |||
char * clientip[MAXCONNS]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might even want to get rid of all of this as we aren't implementing an authentication directly in FTL
now.
main.c
Outdated
pthread_t piholelogthread; | ||
if(pthread_create( &piholelogthread, &attr, pihole_log_thread, NULL ) != 0) | ||
{ | ||
logg("Unable to open Pi-hole log processing thread. Exiting..."); | ||
killed = 1; | ||
} | ||
sleepms(100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this delay is needed at all.
main.c
Outdated
pthread_t socket_listenthread; | ||
if(pthread_create( &socket_listenthread, &attr, socket_listening_thread, NULL ) != 0) | ||
{ | ||
logg("Unable to open Unix socket listening thread. Exiting..."); | ||
killed = 1; | ||
} | ||
sleepms(100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one can be removed as well, I think.
socket.c
Outdated
return accept(sockfd, (struct sockaddr *) &in6_addr, &socklen); | ||
memset(&in6_addr, 0, sizeof(in6_addr)); | ||
socklen = sizeof(un_addr); | ||
char str[INET6_ADDRSTRLEN]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed
socket.c
Outdated
@@ -293,7 +330,7 @@ void *telnet_connection_handler_thread(void *socket_desc) | |||
// Requests should not be processed/answered when data is about to change | |||
enable_thread_lock(threadname); | |||
|
|||
process_request(message, &sock); | |||
process_request(message, &sock, TELNET); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TELNET
is not needed here, as we have istelnet[sock]
socket.c
Outdated
@@ -319,6 +356,11 @@ void *telnet_connection_handler_thread(void *socket_desc) | |||
close(sock); | |||
free(socket_desc); | |||
|
|||
if(clientip[sock] != NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed as well.
socket.c
Outdated
@@ -351,7 +395,7 @@ void *socket_connection_handler_thread(void *socket_desc) | |||
// Requests should not be processed/answered when data is about to change | |||
enable_thread_lock(threadname); | |||
|
|||
process_request(message, &sock); | |||
process_request(message, &sock, SOCKET); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here, SOCKET
can be removed.
socket.c
Outdated
@@ -376,6 +420,11 @@ void *socket_connection_handler_thread(void *socket_desc) | |||
close(sock); | |||
free(socket_desc); | |||
|
|||
if(clientip[sock] != NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove this as well.
msgpack.c
Outdated
// Make sure that the length is less than 4294967296 | ||
size_t length = strlen(string); | ||
|
||
if(length >= 4294967296) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
msgpack.c: In function ‘pack_str32’:
msgpack.c:95:12: warning: comparison is always false due to limited range of data type [-Wtype-limits]
if(length >= 4294967296) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When compiling on a 64 bit system, this warning does not appear. Do you think we should check for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4294967296 is slightly more than 4 GB, do we really expect strings to become that large?
If you want to keep the check (which is probably, a good idea), we can test for the length of size_t
, maybe like this:
if((sizeof(size_t) > 4) && length >= 4294967296)
{ ... }
There shouldn't be any performance penalty here, as with any (non-zero) level of optimization the compiled binary won't actually perform the test as the compiler will already eliminate whichever branch is dead at compile time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That code still gives the compiler warning. How about reducing it to:
if(length >= 2147483648) { ... }
The compiler does not yell about this, but it is a slightly sloppy way to avoid it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot test this at the moment but there should be a preprocessor constant for the maximum allowed size of size_t
. Maybe something like
#if __SIZE_MAX__ > 4294967295
if(length > 4294967295)
{ ... }
#endif
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some minor bugs and possible enhancements that need to be addressed in api.c
but others than that it is nice work!
api.c
Outdated
if(total > 0) | ||
percentage = 1e2*blocked/total; | ||
|
||
// MAX_INT is 10 digits, +1 for the null terminator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need these buffers at all? Instead of all the strncpy()
things we can directly use ssend()
(as it is done in development) to reduce code complexity and avoid unnecessary buffers.
You may want to look at https://github.com/pi-hole/FTL/blob/development/request.c#L245-L253 and https://github.com/pi-hole/FTL/blob/development/request.c#L274-L285 as I think that is nicely done here.
api.c
Outdated
if(istelnet[*sock]) | ||
strncpy(domains_blocked, "N/A", 4); | ||
else | ||
strncpy(domains_blocked, "\"N/A\"", 6); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem that we need this at all (if sending over the Unix socket, we don't use this variable).
api.c
Outdated
{ | ||
int i, j = 9999999; | ||
|
||
// Get first time slot with total or blocked greater than zero (the array will go down over time due to the rolling window) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the array will go down over time due to the rolling window"
I had to read that twice to understand that you want to say that the individual entries will go to zero over time, maybe it should be clarified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you wrote this comment ;)
api.c
Outdated
if(istelnet[*sock]) | ||
ssend(*sock, "%i %i %s wildcard\n", n, domains[j].blockedcount, domains[j].domain); | ||
else { | ||
char *fancyWildcard = calloc(2 + strlen(domains[j].domain), sizeof(char)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will have to allocate 3 + strlen
as you have to store the string, *
, .
and the terminating NULL
(strlen
does not consider the NULL
in the injected string when counting the length).
api.c
Outdated
} | ||
// Domain filtering? | ||
if(command(client_message, ">getallqueries-domain")) { | ||
sscanf(client_message, ">getallqueries-domain %ms", &domainname); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking out aloud: Could this %ms
be a potential DDoS vector here? Like you send FTL
some 100 MB through the telnet socket and fill up the memory? I think this was limited to some certain (fixed) length previously. I'm fine with %ms
, but we should really check for the domainname
pointer afterwards..
The Posix standard documents this extension in its POSIX.1-2008 edition:
The application shall be responsible for freeing the memory after usage. If there is insufficient memory to allocate a buffer, the function shall set
errno
toENOMEM
and a conversion error shall result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The input is limited to 1024 chars, but they could just send the same command over and over again. Then it would only be limited by the max number of connections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is how development currently handles it:
https://github.com/pi-hole/FTL/blob/master/request.c#L680-L685
This version was implemented in this commit:
48d355b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version was implemented in this commit:
48d355b
Is this the nice way of blaming me for it? ;-) I'm not always and only having great ideas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want me to revert it back to the development way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please. Maybe increase it to 256 if you find it necessary.
api.c
Outdated
} | ||
// Client filtering? | ||
if(command(client_message, ">getallqueries-client")) { | ||
sscanf(client_message, ">getallqueries-client %ms", &clientname); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above
api.c
Outdated
{ | ||
for(i = sendit; i < counters.overTime; i++) | ||
{ | ||
double percentage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could go down to float
as we truncate the digits anyhow later on. It would make the case on line api.c:902 unnecessary.
api.c
Outdated
{ | ||
validate_access("overTime", i, true, __LINE__, __FUNCTION__, __FILE__); | ||
|
||
double percentageIPv4 = 0.0, percentageIPv6 = 0.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use float
here as well, removes casts in api.c:965f
api.c
Outdated
sprintf(hashVersion, "vDev-%s", hash); | ||
|
||
pack_str32(*sock, hashVersion); | ||
pack_str32(*sock, (char *) tag); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this cast necessary? See also api.c:985.
I think constants (like GIT_HASH
) are treated as const char*
as they are pointers to a const
location in space where this string is compiled into the binary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a compiler warning otherwise:
api.c: In function 'getVersion':
api.c:1007:22: warning: passing argument 2 of 'pack_str32' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
pack_str32(*sock, tag);
^~~
In file included from api.c:12:0:
api.h:41:6: note: expected 'char *' but argument is of type 'const char *'
void pack_str32(int sock, char *string);
^~~~~~~~~~
api.c
Outdated
if(istelnet[*sock]) | ||
ssend(*sock, "version vDev-%s\ntag %s\nbranch %s\ndate %s\n", hash, tag, GIT_BRANCH, GIT_DATE); | ||
else { | ||
char *hashVersion = calloc(5 + strlen(hash), sizeof(char)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has to be increased by one due to the terminating NULL
character
Signed-off-by: Mcat12 <newtoncat12@yahoo.com>
# Conflicts: # FTL.h # socket.c
Signed-off-by: Mcat12 <newtoncat12@yahoo.com>
Signed-off-by: Mcat12 <newtoncat12@yahoo.com>
Signed-off-by: Mcat12 <newtoncat12@yahoo.com>
Signed-off-by: Mcat12 <newtoncat12@yahoo.com>
Signed-off-by: Mcat12 <newtoncat12@yahoo.com>
Signed-off-by: Mcat12 <newtoncat12@yahoo.com>
Signed-off-by: Mcat12 <newtoncat12@yahoo.com>
Signed-off-by: Mcat12 <newtoncat12@yahoo.com>
Signed-off-by: Mcat12 <newtoncat12@yahoo.com>
Signed-off-by: Mcat12 <newtoncat12@yahoo.com>
Approved 🎉 |
By submitting this pull request, I confirm the following (please check boxes, eg [X]) Failure to fill the template will close your PR:
Please submit all pull requests against the
development
branch. Failure to do so will delay or deny your request10
[Edited by Mcat12]
Change Unix socket interface to return data in
MessagePack
format. It still uses the same API commands as the telnet interface. The MessagePack format allows FTL to efficiently stream data in a very compact form to the upcoming HTTP API.Also included are these smaller changes:
istelnet[]
array of bools.This PR previously held an implementation of an HTTP API, but that will be moved to a separate program. It will access FTL's data through the Unix socket.
This template was created based on the work of
udemy-dl
.