lammertb / libhttp Goto Github PK

The fields must_close, in_error_handler and internal_error seem to be booleans and it would be better to declare them as such.

Not intuitive to have two comparable functions where one is just the other with one missing parameter.

Same issue as with localtime(). A thread safe platform independent function must be created.

This function uses a fake ctx structure which is static and returned in the connection if this function returns. So technically there can only run one client connection at the time.

The function XX_httplib_push_all takes a len parameter of type int64_t, whereas the function push() takes an int parameter which is less than 64 bits on many architectures. A cast hides the potential danger of this.

At the end of the function cdata is freed, but cdata is just a pointer to some externally allocated memory. This is suspicious and the function call may either be legitimate, but in that case also all children of the cdata structure should be examined, or it is a bogus call and should be removed entirely from the code base.

Better to change the return type to bool for compiler type checking purposes.

These should be named with defines or in an enum to make the code readable without head scratching. Errors are very difficult to catch now.

All function calls use the same static structure.

A number of system initialization calls are currently in httplib_start(). This makes it impossible to call the function httplib_start() multiple times for multiple contexts to run independent servers with independent functionality, different DOCUMENT_ROOT etc. One-time system initialization functions must be moved to httplib_system_init() to make it possible to start multiple independent contexts.

As I spent hours trying to get this to compile, I realized that the documentation differs so much from the actual thing that I need to do just to make it work.

Function is in practice a fall through to the pthread_ equivalent on systems which support it.

In some places in the software ctx->config members are first tested against NULL and only afterwards used but in other places these members are used directly without check. This may be cause for memory faults.

The commit bd926d1 (the latest one today nov.18th 2018 on master) fails to build:

cc -c  -Wall -Wextra -Wstrict-prototypes -Wshadow -Wpointer-arith -Wformat-security -Winit-self -Wcast-qual -Wcast-align -Wwrite-strings -Wnested-externs -Wredundant-decls -Werror -O3 -funsigned-char -Iinclude/ -DUSE_STACK_SIZE=102400 -oobj/httplib_ssl_get_client_cert_info.o src/httplib_ssl_get_client_cert_info.c
src/httplib_ssl_get_client_cert_info.c: In function ‘XX_httplib_ssl_get_client_cert_info’:
src/httplib_ssl_get_client_cert_info.c:101:15: error: cast between incompatible function types from ‘int (*)(X509 *, unsigned char **)’ {aka ‘int (*)(struct x509 *, unsigned char **)’} to ‘int (*)(void)’ [-Werror=cast-function-type]
  ASN1_digest( (int (*)(void))i2d_X509, digest, (char *)cert, buf, &ulen );
               ^
cc1: all warnings being treated as errors
make: *** [Makefile:148: obj/httplib_ssl_get_client_cert_info.o] Error 1

This is on Linux/x86-64/Debian/Sid the GCC compiler is gcc version 8.2.0 (Debian 8.2.0-9). The GNU libc is Debian GLIBC 2.27-8. The kernel is Linux version 4.18.0-2-rt-amd64

The access control and netmask calculation functions only work with IPv4 addresses.

In httplib_start() a local variable ok is used to check a number of return codes. The values to this variable are assigned with the &= operator. Unfortunately this is a bit wise AND operator while a boolean AND (&&) should have been used. This can give strange effects, for example:

1 && 2 is TRUE because both values are non-zero
1 & 2 is FALSE because the values do not share any bits

This is a highly dangerous construct and a typical example of knowing just enough to be dangerous.

Having asserts in production code is plain wrong. If rare conditions are not met, functions should stop gracefully and inform the higher calling party about the problem.

Imagine that Linux or Windows had asserts in their code.

Ohh yes, Windows have their BOD......

The block for errno handling in the static push function is very very empty. This is called nice weather code. As long as everything goes fine everybody thinks the program is working. But when it starts raining the problems start.

Changing port number to uint16_t would reduce the number of checks and conversions significantly. It should be checked first however if in some border case negative or large numbers are assigned to the port number for specific undocumented purposes.

The function XX_httplib_worker_thread() frees memory which was passed to it as parameter and which it doesn't own. This assumes that the pointer is valid and was allocated before and not static, and that the memory structure is not complex with other memory blocks to be freed. This may be legitimate or not, but should be investigated.

When preparing the CGI environment, it is possible that the config option DOCUMENT_ROOT is NULL. Security issues may arise if no document root is set in the CGI call. This situation has to be carefully analyzed.

Values of context_type should be converted to defined integers or an enum range.

ws_server example does not build.i .C file also references to libcivetweb.a..

The throttle functionality is triggered based on a uint32_t variable. Therefore it doesn't work with IPv6.

Config options are both recognized by their name, and by constants defined in an enum. This is error prone because both get easily out of sync. This should be changed to lookup by name only.

In 2016, IPv6 is no longer an afterthought but a necessity. Apple tests all their iPhone apps in a IPv6-only environment for example. Therefore IPv6 support should always be compiled in and switched on or off with runtime options rather than compile time options.

I may be off base here, but whenever I find a new library I try to discern the motivation for creating it. Why are you making this library? What does it address that other libraries don't? Not just the technologies, what are the philosophical differences you have with other libraries that led to you creating this library as opposed to contributing to another that you derived from?

By the way, I do love it. I just want other people to be able to enjoy it without prejudice.

In httplib_connect_websocket_client() a variable handshake_req is used which is declared static. The contents of this variable changes with every call depending on a parameter. This is not thread safe because the value of this variable may change unexpectedly and there also doesn't seem to be a reason why this should be a static variable.

Has to be fixed after further investigation.

Just curious about the performance of this. I write a lot of code in Go and there is a bunch of http packages and all of them provide benchmarks mostly to let others know that when choosing an http package which one best suites their needs.

So my thought is if you provided some benchmarks that might help people decide if its worth this over others that exist out there.

The makefile defaults to MSVC tools when Windows is detected, maybe there is a way to make it detect if its running in MSYS2 and switch to GCC automatically?

When preparing a CGI environment, AUTHENTICATION_DOMAIN can be NULL. For security reasons it must be carefully analyzed what the implications are if no server value is passed to the CGI script.

Better to switch the return type to bool to enhance compiler type checking

Hi Lammertb,

I just want to know if libhttp supports FreeRTOS + LWIP environment.
I am trying to make a http server on Zynq 7000 SoC chip and
am using FreeRTOS with LWIP.
Would you mind telling me how to compile and run on FreeRTOS ?
Thank you in advance,

kevin

In httplib_free_context() runs a for loop over all ctx->config options to free them. This part already has a MSVC warning 6001 disabled. This looks fishy because it is not guaranteed that all memory blocks in the config block have been allocated from the heap before.

SSI code can load files without a DOCUMENT_ROOT set. This more or less negates the idea of the NO_FILES option, because you can still serve files through SSI files. The NO_FILES option therefore only seems to be effective for static furniture like images etc. HTML can still be servered through .shtml files.

This is confusing and needs a proper solution.

Windows error codes are retrieved with GetLastError(), but the human readable text for these error codes is retrieved with the Posix type strerror() which assumes errno error codes. The error codes do not match so in practice you won't see any human readable errors in a Windows installation.

This must be fixed.

The maximum depth of ssi includes is a fixed value in send_ssi_file, while it is a configuration option in other places.

The Windows CE implementation of localtime() uses an array to store the return values of the function. This array can store the same amount of values as LibHTTP starts worker threads. If the main application however also uses the LibHTTP provided localtime function in additional threads created outside of the scope of LibHTTP, this may cause overwriting the values belonging to other threads.

Thread local storage should be used in this function, or better, all calls to localtime() should be replaced to calls to localtime_s() which uses storage provided by the calling thread.

The same problem is also existing in the Windows CE implementation for gmtime() which is the UTC equivalent of localtime().

Hello,
I am encountering an error when trying to build. This appears to be a case of warning treated as error. Do the build instructions still hold good? (I tried make build and it gave me an error)

Issue 1:
Build issue on ubuntu

$ uname -a
Linux ubuntu 4.13.0-16-generic #19-Ubuntu SMP Wed Oct 11 18:35:14 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

$ make
cc -c -Wall -Wextra -Wstrict-prototypes -Wshadow -Wpointer-arith -Wformat-security -Winit-self -Wcast-qual -Wcast-align -Wwrite-strings -Wnested-externs -Wredundant-decls -Werror -O3 -funsigned-char -Iinclude/ -DUSE_STACK_SIZE=102400 -oobj/httplib_error_string.o src/httplib_error_string.c
src/httplib_error_string.c: In function ‘httplib_error_string’:
src/httplib_error_string.c:170:2: error: ignoring return value of ‘strerror_r’, declared with attribute warn_unused_result [-Werror=unused-result]
strerror_r( error_code, buf, buf_len );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
Makefile:148: recipe for target 'obj/httplib_error_string.o' failed
make: *** [obj/httplib_error_string.o] Error 1

Issue-2:
on MAC the build failed with a different error:

$ make
cc -c -Wall -Wextra -Wstrict-prototypes -Wshadow -Wpointer-arith -Wformat-security -Winit-self -Wcast-qual -Wcast-align -Wwrite-strings -Wnested-externs -Wredundant-decls -Werror -O3 -funsigned-char -Iinclude/ -DUSE_STACK_SIZE=102400 -oobj/extern_sha1.o src/extern_sha1.c
In file included from src/extern_sha1.c:28:
src/httplib_main.h:130:9: error: 'CLOCK_MONOTONIC' macro redefined [-Werror,-Wmacro-redefined]
#define CLOCK_MONOTONIC (1)
^
/usr/include/time.h:156:9: note: previous definition is here
#define CLOCK_MONOTONIC _CLOCK_MONOTONIC
^
In file included from src/extern_sha1.c:28:
src/httplib_main.h:131:9: error: 'CLOCK_REALTIME' macro redefined [-Werror,-Wmacro-redefined]
#define CLOCK_REALTIME (2)
^
/usr/include/time.h:154:9: note: previous definition is here
#define CLOCK_REALTIME _CLOCK_REALTIME
^
2 errors generated.
make: *** [obj/extern_sha1.o] Error 1

$ make -f Makefile.osx
cc -c -Wall -Wextra -Wstrict-prototypes -Wshadow -Wpointer-arith -Wformat-security -Winit-self -Wcast-qual -Wcast-align -Wwrite-strings -Wnested-externs -Wredundant-decls -Werror -O3 -funsigned-char -Iinclude/ -DUSE_STACK_SIZE=102400 -oobj/extern_sha1.o src/extern_sha1.c
In file included from src/extern_sha1.c:28:
src/httplib_main.h:130:9: error: 'CLOCK_MONOTONIC' macro redefined [-Werror,-Wmacro-redefined]
#define CLOCK_MONOTONIC (1)
^
/usr/include/time.h:156:9: note: previous definition is here
#define CLOCK_MONOTONIC _CLOCK_MONOTONIC
^
In file included from src/extern_sha1.c:28:
src/httplib_main.h:131:9: error: 'CLOCK_REALTIME' macro redefined [-Werror,-Wmacro-redefined]
#define CLOCK_REALTIME (2)
^
/usr/include/time.h:154:9: note: previous definition is here
#define CLOCK_REALTIME _CLOCK_REALTIME
^
2 errors generated.
make: *** [obj/extern_sha1.o] Error 1

Type of the parameter should be changed looking at the functionality of it.

During startup of a server with a call to httplib_start(), there are many reasons why the start could potentially fail. Unfortunately error reporting back to the calling party is almost not existent. Only some httplib_cry() calls are in place, but these report to a file which may not be opened due to setting or other errors. Also Windows socket error codes are not compatible with Linux, and these errors are reported with an error number followed by "no error".

Error handling during startup must be better and reporting errors back to the application should be made much more friendly.

Hello,
I am following the building guide doc/Build.md to build the libhttp with socket enable. But I found
none of the building target works.

issue 1:
None of make option works. It works only with 'make' in the command and removing Werror in Makefile. I found the there is no target, help, build..., in the Makefile

$ make lib WITH_WEBSOCKET=1
make: Nothing to be done for 'lib'.

$ make build
make: *** No rule to make target 'build'. Stop.

$ make help
make: *** No rule to make target 'help'. Stop.

issue 2:
I am trying to build websocket_client in examples/websocket_client. Weird thing is that it removes the all libs and tries to copy libcivetweb.a which I cannot find this word(libcivetweb) in any file except all Makefile in the example directory.

examples/websocket_client$ make

make -C ../.. clean lib WITH_WEBSOCKET=1
make[1]: Entering directory '/home/tyler/Documents/project/libhttp'
/bin/rm -f obj/*.o
/bin/rm -f lib/libhttp.a
/bin/rm -f testmime
/bin/rm -f libhttpserver
make[1]: *** No rule to make target 'lib'. Stop.
make[1]: Leaving directory '/home/tyler/Documents/project/libhttp'
Makefile:31: recipe for target 'libcivetweb.a' failed
make: *** [libcivetweb.a] Error 2

Lot of locking mess in this function probably to allow multiple threads to call non-threadsafe calls to OpenSSL.

Two static variables to store the state of two random generators. It won't hurt much probably because the outcome of the generator is "random" anyway, but we have to check if this is the best option.

Currently error messages are either logged or discarded. It would be better to sort them in groups like informational, warning, error and failure and switch logging of them on and off with run time settings. Also grouping of errors based on subsystem (socket errors, file errors, general errors, etc) could be possible to give the application further control about the messages they will receive or ignore.

Does it support webrtc? If no is there any plan to do so?

There are uses for such a random generator on the application level. So why not make it public as a utility function?

In this function first a field is_master is set to -3, and the next statement is a call to free the memory.