Portability fixes

[Zer0-One]

• CFLAG gnu99 was changed to c99.
• CXXFLAG c++98 was changed to c++11.
• CFLAG -pedantic-errors was added so that non-ISO C now throws errors.
• _XOPEN_SOURCE feature test macro added and set to 600 to expose SUSv3
  and c99 definitions in modules that required them.
• Fixed tests (and bootstrap daemon logging) that were failing due to
  the altered build flags.
• Avoid string suffix misinterpretation; explicit narrowing conversion.
• Misc. additions to .gitignore to make sure build artifacts don't wind
  up in version control.

I didn't try running the tests, but everything seems to build just fine now. If someone could test for me, I'd really appreciate it.

---

This change is 

[iphydf]

Run other/astyle/format-source.

[iphydf]

Warning suppressions were removed

Indeed, and now it no longer compiles. Please add them back.

[GrayHatter]

Reviewed 1 of 5 files at r1.
Review status: 1 of 5 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

.gitignore, line 25 at r1 (raw file):

DartConfiguration.tcl
CTestTestfile.cmake
*.pc

these aren't make files, they're generated files

---

CMakeLists.txt, line 73 at r1 (raw file):

  # -pedantic only for C, because in C++ we want to allow the GNU/C99 extension
  # of having a comma at the end of an enumerator list.
  add_cflag("-pedantic")

why did you drop -pedantic

---

testing/misc_tools.c, line 32 at r1 (raw file):

#include <stdlib.h>
#include <string.h>
#include <strings.h>

what are you including strings.h for?

---

testing/tox_sync.c, line 319 at r1 (raw file):

                    memcpy(filepath+strlen(path), dir->d_name, strlen(dir->d_name)+1);
                    stat(filepath, &statbuf);
                    if (S_ISREG(statbuf.st_mode)) {

why is this better?

---

Comments from Reviewable

[Zer0-One]

these aren't make files, they're generated files

I know that, but does it really matter? What heading do you want me to put them under?

why did you drop -pedantic

Because I added -pedantic-errors, which does the same and more.

what are you including strings.h for?

Because that's where strncasecmp() is declared

why is this better?

Because d_type is a BSD-ism, and the point of this PR is to increase portability.

There's a little bit more for me to fix up to get it to build, pls no mergerino por favorino. I'll let you guys know when it's good.

[Zer0-One]

@GrayHatter good now?

[iphydf]

Review status: 1 of 6 files reviewed at latest revision, 7 unresolved discussions.

---

CMakeLists.txt, line 60 at r3 (raw file):

endmacro()
  
# Set standard version for compiler

Add the '.' back here and add them to the comments below.

---

CMakeLists.txt, line 68 at r3 (raw file):

# Expose SUSv3 and C99 definitions
add_definitions(-D_XOPEN_SOURCE=600)

Can you add this definition to the files that actually need it rather than the command line?

---

other/bootstrap_daemon/src/log.c, line 119 at r3 (raw file):

        case LOG_BACKEND_STDOUT:
            fprintf(level_stdout(level), buf);

This probably doesn't work. Where are the args?

---

Comments from Reviewable

[iphydf]

Review status: 1 of 6 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

other/bootstrap_daemon/src/log.c, line 119 at r3 (raw file):

Previously, iphydf wrote…

This probably doesn't work. Where are the args?

Ah. I see now. Don't use fprintf with a user-specified format string.

---

Comments from Reviewable

[nurupo]

Review status: 1 of 23 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

other/bootstrap_daemon/src/log.c, line 119 at r3 (raw file):

Previously, iphydf wrote…

Ah. I see now. Don't use fprintf with a user-specified format string.

The "don't use user-specified format string" is supposed to protect against the format string attacks, isn't it? But all the format strings that we pass to the logging procedure are not really user-specified, they are compile-time specified, the actual user of tox-bootstrapd binary can't alter the format, neither the format string comes from other potentially insecure input sources such as the network. It is user-specified only from the point of the view of the logging module, as in "the rest of the code is the user". Even if we ignore the fact that the string is not really user-specified and for a second assume that it is (it's not though), now we use vsnprintf with as such "user-specified format string", so how is that an improvement? Am I missing something?

---

testing/tox_sync.c, line 319 at r4 (raw file):

                    char filepath[strlen(path) + strlen(dir->d_name)];
                    memcpy(filepath, path, strlen(path));
                    memcpy(filepath + strlen(path), dir->d_name, strlen(dir->d_name) + 1);

So, you allocate a buffer of size X+Y but you write X+Y+1 into it?

---

Comments from Reviewable

[nurupo]

other/bootstrap_daemon/src/log.c, line 119 at r3 (raw file):

use vsnprintf with as such

use vsnprintf with such*

---

Comments from Reviewable

[Zer0-One]

@nurupo good catch, i didn't include space for the terminating null.

[nurupo]

@Zer0-One open Reviewable and comment in it. You have to resolve the discussions and such.

[Zer0-One]

Resolved

---

Review status: 1 of 25 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

.gitignore, line 25 at r1 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

these aren't make files, they're generated files

We'll clean this up in a different PR, where we shove build artifacts into a directory that isn't the root, and then just keep that untracked by git.

---

CMakeLists.txt, line 73 at r1 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

why did you drop -pedantic

Because I added -pedantic-errors

---

CMakeLists.txt, line 60 at r3 (raw file):

Previously, iphydf wrote…

Add the '.' back here and add them to the comments below.

done

---

CMakeLists.txt, line 68 at r3 (raw file):

Previously, iphydf wrote…

Can you add this definition to the files that actually need it rather than the command line?

done

---

testing/misc_tools.c, line 32 at r1 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

what are you including strings.h for?

Because that's where strncasecmp() is declared

---

testing/tox_sync.c, line 319 at r1 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

why is this better?

Because d_type is a BSD-ism

---

testing/tox_sync.c, line 319 at r4 (raw file):

Previously, nurupo wrote…

So, you allocate a buffer of size X+Y but you write X+Y+1 into it?

good catch

---

Comments from Reviewable

[sudden6]

took a quick look, to me

---

Review status: 1 of 25 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[iphydf]

@Zer0-One I've pushed a commit. Please squash it into yours if you are ok with that change.

[iphydf]

Review status: 1 of 26 files reviewed at latest revision, 7 unresolved discussions.

---

CMakeLists.txt, line 60 at r3 (raw file):

Previously, Zer0-One (David Zero) wrote…

done

Not really done. Add . to the comment below as well.

---

other/bootstrap_daemon/src/log.c, line 119 at r3 (raw file):

Previously, nurupo wrote…

use vsnprintf with as such

use vsnprintf with such*

write_log(level, "hello %s", "world%s"); what happens if instead of fprintf(level_stdout(level), "%s", buf), this said fprintf(level_stdout(level), buf) as it did before?

---

Comments from Reviewable

[Zer0-One]

rebased onto latest master

---

Review status: 11 of 27 files reviewed at latest revision, 7 unresolved discussions.

---

other/bootstrap_daemon/src/log.c, line 91 at r7 (raw file):

Previously, nurupo wrote…

>= actually, i.e. that it's non-negative.

lol whoops. fixed

---

Comments from Reviewable

[GrayHatter]

Reviewed 2 of 5 files at r1, 4 of 20 files at r4, 3 of 8 files at r5, 10 of 13 files at r7, 6 of 6 files at r8.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[GrayHatter]

Reviewed 1 of 8 files at r5.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[GrayHatter]

[iphydf]

@nurupo is this done?

[nurupo]

Reviewed 11 of 13 files at r7, 6 of 6 files at r8.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

other/bootstrap_daemon/src/log.c, line 88 at r8 (raw file):

    va_copy(args2, args);
    int size = vsnprintf(NULL, 0, format, args2) + 1;

I just noticed this + 1 at the end. It breaks the checks we do in assert and if below, since vsnprintf() returns a negative when it errors, which might be -1. Also, I don't see any need in having +1 in there technically speaking, as you already account for vsnprintf() not counting null byte by having +1 in buf[size + 1] below.

I think this +1 should be removed.

---

Comments from Reviewable

[Zer0-One]

finished. rebased onto latest master.

---

Review status: 25 of 27 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

other/bootstrap_daemon/src/log.c, line 88 at r8 (raw file):

Previously, nurupo wrote…

I just noticed this + 1 at the end. It breaks the checks we do in assert and if below, since vsnprintf() returns a negative when it errors, which might be -1. Also, I don't see any need in having +1 in there technically speaking, as you already account for vsnprintf() not counting null byte by having +1 in buf[size + 1] below.

I think this +1 should be removed.

you're right, removed.

---

Comments from Reviewable

[nurupo]

Reviewed 1 of 2 files at r9.
Review status: 26 of 27 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

other/bootstrap_daemon/src/log.c, line 98 at r9 (raw file):

    char buf[size + 1];
    vsnprintf(buf, size, format, args);

Ok, found another issue. The size you pass to vsnprintf() as the second argument is 1 byte short. What about using sizeof(buf) as the second argument?

---

Comments from Reviewable

[GrayHatter]

Reviewed 1 of 2 files at r9.
Review status: 26 of 27 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[GrayHatter]

Review status: 26 of 27 files reviewed at latest revision, 3 unresolved discussions.

---

other/bootstrap_daemon/src/log.c, line 119 at r3 (raw file):

Previously, iphydf wrote…

Never mind. I don't care. As long it works, I'm fine with this.

@iphydf you need to okay this to clear reviweable

---

other/bootstrap_daemon/src/log.c, line 98 at r9 (raw file):

Previously, nurupo wrote…

Ok, found another issue. The size you pass to vsnprintf() as the second argument is 1 byte short. What about using sizeof(buf) as the second argument?

or init size like int size = 1 + vsnprintf(...);

then change the if to be <= and you can simply use size.

---

Comments from Reviewable

[GrayHatter]

Reviewed 1 of 2 files at r9.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[iphydf]

Reviewed 3 of 5 files at r1, 4 of 20 files at r4, 4 of 8 files at r5, 10 of 13 files at r7, 4 of 6 files at r8, 2 of 2 files at r9.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[iphydf]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

other/bootstrap_daemon/src/log.c, line 119 at r3 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

@iphydf you need to okay this to clear reviweable

Ack.

---

Comments from Reviewable

[GrayHatter]

---

Reviewed 1 of 1 files at r10.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[Zer0-One]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

other/bootstrap_daemon/src/log.c, line 98 at r9 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

or init size like int size = 1 + vsnprintf(...);

then change the if to be <= and you can simply use size.

fixed, in what i think is the most readable way.

---

Comments from Reviewable

[iphydf]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

CMakeLists.txt, line 69 at r10 (raw file):

option(WARNINGS "Enable additional compiler warnings" ON)
if(WARNINGS)
# Add all warning flags we can.

Indent comments with 2 spaces just like the rest of the block.

---

Comments from Reviewable

[Zer0-One]

Review status: 26 of 27 files reviewed at latest revision, 2 unresolved discussions.

---

CMakeLists.txt, line 69 at r10 (raw file):

Previously, iphydf wrote…

Indent comments with 2 spaces just like the rest of the block.

done

---

other/bootstrap_daemon/src/log.c, line 98 at r9 (raw file):

Previously, Zer0-One (David Zero) wrote…

fixed, in what i think is the most readable way.

Done.

---

Comments from Reviewable

[nurupo]

---

Reviewed 1 of 1 files at r10, 2 of 2 files at r11.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[GrayHatter]

---

Reviewed 2 of 2 files at r11.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable