Call of ONION_DEBUG(...) changes input argument

[YggdrasiI]

Hello,

on my Android device with Termux (Linux 4.4.156-R3+ aarch64 Android), I've got a strange bug by using otemplate. After some investigation I found out that ONION_DEBUG(const char str) changes its input string argument. I didn't found a solution yet, but if otemplate is affected this bug might also occur at other calls of the macro. I was not able to reproduce the problem on my laptop.

The position where the bug occurs: tools/otemplate/otemplate.c

184   const char *tname = basename(tmp2);
185   ONION_DEBUG("Create init function on top, tname %s", tname);

After line 184, tname is set correctly to 'some_filename.html'. After line 185, tname is set on 'otemplate.c'.

Commenting out this line in _onion_logstderr (src/onion/log.c)

138   filename = basename((char *)filename);

fixes the issue. So maybe some string pointers share the same memory and POSIX's basename() function is called instead of GNU's basename()? Importing of triggers the usage of the first variant.

Regards YggdrasiI

---

Steps to reproduce the problem: I compiled everything directly on the device. Maybe it also occurs if the Android NDK will used for cross compiling.

• Install Termux on Android Smartphone • in Termux: apt install git cmake gcc (note that Termux will install clang instead of gcc) • Setup sshd in Termux or use Adb Shell to avoid typing this stuff directly on the device… • Mkdir '$HOME/includes' and copy Android Header execinfo.h into this folder • Clone onion and Insert the following part into CMakeLists.txt

if("${CMAKE_SYSTEM_NAME}" EQUAL "Android")  # True in Termux
       # Add folder with execinfo.h
       include_directories(SYSTEM "$HOME/include")

       # Link extra library log on all targets
       macro (add_executable _name)
               # invoke built-in add_executable
               _add_executable(${ARGV})
               if (TARGET ${_name})
                       target_link_libraries(${_name} log)
               endif()
       endmacro()
endif("${CMAKE_SYSTEM_NAME}" EQUAL "Android")

Build minimal version of onion with Debug-Build-Type

mkdir build 
                cd build && cmake \
                -DCMAKE_BUILD_TYPE=Debug \
                -DONION_USE_SSL=false \
                -DONION_USE_PAM=false \
                -DONION_USE_PTHREADS=false \
                -DONION_USE_PNG=false \
                -DONION_USE_JPEG=false \
                -DONION_USE_XML2=false \
                -DONION_USE_SYSTEMD=false \
                -DONION_USE_SQLITE3=false \
                -DONION_USE_REDIS=false \
                -DONION_USE_GC=false \
                -DONION_USE_TESTS=false \
                -DONION_EXAMPLES=false \
                -DONION_USE_BINDINGS_CPP=false \
                -DONION_POLLER=epoll \
                .. \
    && make

And finally, use otemplate to generate a file

./build/tools/otemplate/otemplate examples/ofileserver/fileserver.html fileserver_html.c

This results in an output file with some wrong function names, i.e.

onion_connection_status otemplate_c_handler_page(onion_dict context, onion_request req, onion_response *res){

Correct would be:

onion_connection_status fileserver_html_handler_page(onion_dict context, onion_request req, onion_response *res){

[davidmoreno]

From my initial understanding the problem is more on the basename(3) function than in onion. It might be that it always returns a pointer to the same statically allocated memory for the path, and calling again basename returns the same pointer, but modifies the data.

Checking the manpage (https://linux.die.net/man/3/basename) talks abaout some problems and diferences between the POSIX and GNU implementations.

About solutions we could use our own basename, which should not be a problem, and avoid this problem.

About the second part you that you say about otemplate_c and fileserver_html, the problem is that this behaviour is triggering and generating the wrong names, isnt it?

[YggdrasiI]

From my initial understanding the problem is more on the basename(3) function than in onion. It might be that it always returns a pointer to the same statically allocated memory for the path, and calling again basename returns the same pointer, but modifies the data.

I think you're right. So interleaving handling of the return value of basename() leads to the issue.

About solutions we could use our own basename, which should not be a problem, and avoid this problem.

Others solutions would be A) cpy = strdup(basename(foo)); [...]; free(cpy) if an onion logging function follows. B) Use an own basename() in the onion logging function only. Maybe we could simply 'swap' the output position of POSIX's basename() before we generate the logging string and swap back, after?

B had the advantage that users still not need to free the returning pointer of basename() in their code base.

About the second part you that you say about otemplate_c and fileserver_html, the problem is that this behaviour is triggering and generating the wrong names, isnt it?

Yes, with the wrong names, some examples, i.e. ofileserver, breaks during the linking.

[YggdrasiI]

Just for reference: The 'normal' libgen.h basename The Android Bionic variant.

The second file shows the usage of a static char array, which will hold the filename of the latest basename() call. The first file answers your comment

   // I dont know why basename is char *. Please somebody tell me.

If the path ends with '\' the input argument will be changed.

[YggdrasiI]

I send in an possible fix #264 I assume this is not totally fine with your coding conventions because i included the xpg_basename.c file in log.c. Probably you want move the file into an other directory (or reject the pull request).

Regards

[davidmoreno]

Sorry I did not see your pull request, and went ahead and implemented a fix. Can you try it?

about your code, it looks more complex, as it handles much better the end with / case. In my version this would return an empty string. But for the internal uses its OK. Also xdg version is LGPL which I don't think its OK to add to Apache2 licensed code.

If it works for you, can you close the issue?

[YggdrasiI]

Empty strings seems to be the price if we want omit writing into input variables, allocate new memory,or use a static array for the return value. For internal usage of onion_basename(), I had no complains.

Thanks for your effort.