[Patch] nsock/tests/tests_main.c

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

[Patch] nsock/tests/tests_main.c

Gisle Vanem-2
I do like Nmap and will try to contribute. Here is a small fix for now:

--- SVN-Latests/nsock/tests/tests_main.c    2013-06-27 15:25:14 +0000
+++ nsock/tests/tests_main.c 2013-07-11 11:55:05 +0000
@@ -64,7 +64,7 @@
     fflush(stdout);
     rc = test_case_run(current);
     if (rc) {
-      printf(TEST_FAILED " (%s)\n", strerror(-rc));
+      printf(TEST_FAILED " (%s)\n", socket_strerror(-rc));
       break;
     }
     printf(TEST_OK "\n");

----------

Using strerror() for Winsock failures is a big no-no.
Besides, this test program uses ANSI codes to print to the console.
Also a no-no.

--gv
_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] nsock/tests/tests_main.c

Henri Doreau
2013/8/6 Gisle Vanem <[hidden email]>:

> I do like Nmap and will try to contribute. Here is a small fix for now:
>
> --- SVN-Latests/nsock/tests/tests_main.c    2013-06-27 15:25:14 +0000
> +++ nsock/tests/tests_main.c 2013-07-11 11:55:05 +0000
> @@ -64,7 +64,7 @@
>     fflush(stdout);
>     rc = test_case_run(current);
>     if (rc) {
> -      printf(TEST_FAILED " (%s)\n", strerror(-rc));
> +      printf(TEST_FAILED " (%s)\n", socket_strerror(-rc));
>       break;
>     }
>     printf(TEST_OK "\n");
>
> ----------
>
> Using strerror() for Winsock failures is a big no-no.
> Besides, this test program uses ANSI codes to print to the console.
> Also a no-no.
>
> --gv
Thanks!

I've committed the socket_strerror() part with a little change
(r31677). Can you elaborate on why ANSI codes should be avoided? I
wouldn't put them everywhere but they're something quite common for
such tests.

Regards

--
Henri
_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] nsock/tests/tests_main.c

Gisle Vanem-2
"Henri Doreau" <[hidden email]> wrote:

> I've committed the socket_strerror() part with a little change
> (r31677). Can you elaborate on why ANSI codes should be avoided? I
> wouldn't put them everywhere but they're something quite common for
> such tests.

On machines where you have an ANSI-driver in the console, yes.
The Windows console doesn't have that. Running this here now
produces these codes verbatim on the console (not colour changes):

g:\MingW32\src\inet\nmap\nsock\tests> tests_main.exe
nsock pool user data                            [←[1m←[32mOK←[0m]
test timer operations                           [←[1m←[32mOK←[0m]
set standard log levels                         [←[1m←[32mOK←[0m]
check error log levels                          [←[1m←[32mOK←[0m]
simple tcp connection                           Socket troubles: No error
Assertion failed: sd >= 0, file engine_select.c, line 353

The assert() is also a non-portable issue with WinPcap; it doesn't
have a selectable pcap structure or something.

Back to the colours on Windows. tests_main.c could have some
macros like:

#ifdef WIN32
  #define BOLDGREEN (FOREGROUND_GREEN | FOREGROUND_INTENSITY)

  /* default WinCon state */
  static CONSOLE_SCREEN_BUFFER_INFO csbi;
  static void init_console (void)
  {
   ...
  }
  #define PRINT_OK() \
     SetConsoleTextAttribute (stdout_hnd, (csbi.wAttributes & 0xf0) | BOLDGREEN); \
     printf ("OK\n"); \
     SetConsoleTextAttribute (stdout_hnd, csbi.wAttributes)
#else
  #define PRINT_OK() printf(TEST_OK "\n");
#endif

And use PRINT_OK() instead of printf(TEST_OK "\n").
Similar for TEST_FAILED. I could make a patch if there's an interest.

--gv

_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] nsock/tests/tests_main.c

Gisle Vanem-2
"Gisle Vanem" <[hidden email]> wrote:

> simple tcp connection                           Socket troubles: No error
> Assertion failed: sd >= 0, file engine_select.c, line 353
>
> The assert() is also a non-portable issue with WinPcap; it doesn't
> have a selectable pcap structure or something.

No it wasn't that. Simply that WSAStartup() wasn't called. That could
go into a local win_init() that sets up the WinConsole.

BTW. the nsock/tests/*.c files has stuff like:

const struct test_case TestConnectTCP = {
   .t_name     = "simple tcp connection",
   .t_setup    = connect_setup,
   .t_run      = connect_tcp,
   .t_teardown = connect_teardown
};

This C99 (?) feature doesn't work with my MSVC v16.
(it works for MingW-gcc though). Would it hurt that we rewrite this
as:

const struct test_case TestConnectTCP = {
  /* .t_name     = */ "simple tcp connection",
  /* .t_setup    = */ connect_setup,
  /* .t_run      = */ connect_tcp,
  /* .t_teardown = */ connect_teardown
};


--gv
_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] nsock/tests/tests_main.c

Gisle Vanem-2
In reply to this post by Gisle Vanem-2
"Gisle Vanem" <[hidden email]> wrote:

> Back to the colours on Windows. tests_main.c could have some
> macros like:

Ok. I've made an update on tests_main.c for Windows. I'm able
to do a 'sh run_tests.sh'. Which produces only "OK" and in colours
on Windows. Tested with  these shells: 4NT, cmd and Msys' bash interactive.
See attached picture (4NT).

The diff is a little big. So I'm attaching the test_main.c instead for review.

--gv

_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/

run_tests-result.png (22K) Download Attachment
tests_main.c (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] nsock/tests/tests_main.c

Gisle Vanem-2
In reply to this post by Henri Doreau
"Henri Doreau" <[hidden email]> wrote:

> I've committed the socket_strerror() part with a little change
> (r31677).

Here is another misuse of perror(), i.e. strerror(). Which doesn't
print network errors on Windows. Since there's no socket_perror(),
I suggest this instead:

--- SVN-Latest/nsock/src/nsock_connect.c   2013-08-01 11:37:22 +0000
+++ nsock/src/nsock_connect.c        2013-08-07 11:16:43 +0000
   /* inheritable_socket is from nbase */
   iod->sd = (int)inheritable_socket(family, type, proto);
   if (iod->sd == -1) {
-    perror("Socket troubles");
+    printf("Socket troubles %s", socket_strerror(socket_errno()));
     return -1;
   }

----------

--gv
_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] nsock/tests/tests_main.c

Henri Doreau
In reply to this post by Gisle Vanem-2
2013/8/7 Gisle Vanem <[hidden email]>:

> "Henri Doreau" <[hidden email]> wrote:
>
>> I've committed the socket_strerror() part with a little change
>> (r31677). Can you elaborate on why ANSI codes should be avoided? I
>> wouldn't put them everywhere but they're something quite common for
>> such tests.
>
>
> On machines where you have an ANSI-driver in the console, yes.
> The Windows console doesn't have that. Running this here now
> produces these codes verbatim on the console (not colour changes):
>
> g:\MingW32\src\inet\nmap\nsock\tests> tests_main.exe
> nsock pool user data                            [←[1m←[32mOK←[0m]
> test timer operations                           [←[1m←[32mOK←[0m]
> set standard log levels                         [←[1m←[32mOK←[0m]
> check error log levels                          [←[1m←[32mOK←[0m]
> simple tcp connection                           Socket troubles: No error
> Assertion failed: sd >= 0, file engine_select.c, line 353
>
> The assert() is also a non-portable issue with WinPcap; it doesn't
> have a selectable pcap structure or something.
>
> Back to the colours on Windows. tests_main.c could have some
> macros like:
>
> #ifdef WIN32
>  #define BOLDGREEN (FOREGROUND_GREEN | FOREGROUND_INTENSITY)
>
>  /* default WinCon state */
>  static CONSOLE_SCREEN_BUFFER_INFO csbi;
>  static void init_console (void)
>  {
>   ...
>  }
>  #define PRINT_OK() \
>     SetConsoleTextAttribute (stdout_hnd, (csbi.wAttributes & 0xf0) |
> BOLDGREEN); \
>     printf ("OK\n"); \
>     SetConsoleTextAttribute (stdout_hnd, csbi.wAttributes)
> #else
>  #define PRINT_OK() printf(TEST_OK "\n");
> #endif
>
> And use PRINT_OK() instead of printf(TEST_OK "\n").
> Similar for TEST_FAILED. I could make a patch if there's an interest.
>
>
> --gv

Thanks, fixed in r31734 (using a slightly different patch though).

--
Henri
_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] nsock/tests/tests_main.c

Henri Doreau
In reply to this post by Gisle Vanem-2
Hello,

2013/8/7 Gisle Vanem <[hidden email]>:

> "Gisle Vanem" <[hidden email]> wrote:
>
>> simple tcp connection                           Socket troubles: No error
>> Assertion failed: sd >= 0, file engine_select.c, line 353
>>
>> The assert() is also a non-portable issue with WinPcap; it doesn't
>> have a selectable pcap structure or something.
>
>
> No it wasn't that. Simply that WSAStartup() wasn't called. That could
> go into a local win_init() that sets up the WinConsole.
>
Could you send it as a patch?

> BTW. the nsock/tests/*.c files has stuff like:
>
> const struct test_case TestConnectTCP = {
>   .t_name     = "simple tcp connection",
>   .t_setup    = connect_setup,
>   .t_run      = connect_tcp,
>   .t_teardown = connect_teardown
> };
>
> This C99 (?) feature doesn't work with my MSVC v16. (it works for MingW-gcc
> though). Would it hurt that we rewrite this
> as:
>
> const struct test_case TestConnectTCP = {
>  /* .t_name     = */ "simple tcp connection",
>  /* .t_setup    = */ connect_setup,
>  /* .t_run      = */ connect_tcp,
>  /* .t_teardown = */ connect_teardown
>
> };
>
>
> --gv
I know, and it's a pity because these designated initializers are so
nice... But can the nsock test suite be compiled using MSVC? There's
no project file nor anything. I'd be fine changing it if someone works
on it, but I'd rather have a use case when making platform-related
changes.

Regards

--
Henri
_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] nsock/tests/tests_main.c

Gisle Vanem-2
"Henri Doreau" <[hidden email]> wrote:

>> No it wasn't that. Simply that WSAStartup() wasn't called. That could
>> go into a local win_init() that sets up the WinConsole.
>>
> Could you send it as a patch?

Attached. Also, ncat/test/addrset.c suffers the same problem.
Also attached.

> I know, and it's a pity because these designated initializers are so
> nice... But can the nsock test suite be compiled using MSVC? There's
> no project file nor anything.

I've made a project + makefile for it. But this .sln-file will have to
wait.

> +/* socket_strerror() comes from nbase
> + * Declared here to work around a silly inclusion issue until I can fix it. */
> +char *socket_strerror(int errnum);

What's problem including <nbase.h> in "test-common.h"?

--gv

_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/

tests_main.diff (2K) Download Attachment
addrset.diff (643 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] nsock/tests/tests_main.c

Henri Doreau
2013/8/11 Gisle Vanem <[hidden email]>:

> "Henri Doreau" <[hidden email]> wrote:
>
>>> No it wasn't that. Simply that WSAStartup() wasn't called. That could
>>> go into a local win_init() that sets up the WinConsole.
>>>
>> Could you send it as a patch?
>
>
> Attached. Also, ncat/test/addrset.c suffers the same problem.
> Also attached.
>
Why do they both initialize winsock with different version numbers?

Does the simple patch attached sound good to you? I've stripped down
the ANSI colors part. On trunk I simply print results without anything
fancy when running on windows. They're not important and I'd like to
keep it very simple. I'd actually rather use no colors at all than
have complex platform-specific initialization code just for that.

>
>> I know, and it's a pity because these designated initializers are so
>> nice... But can the nsock test suite be compiled using MSVC? There's
>> no project file nor anything.
>
>
> I've made a project + makefile for it. But this .sln-file will have to
> wait.
>
It'd be nice!

>> +/* socket_strerror() comes from nbase
>> + * Declared here to work around a silly inclusion issue until I can fix
>> it. */
>> +char *socket_strerror(int errnum);
>
>
> What's problem including <nbase.h> in "test-common.h"?
>
> --gv
I get double declaration errors which I need to investigate, it's
probably nothing big. I find it ugly as hell but had no time to clean
it since I noticed.

Regards

--
Henri

_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/

nsock_test_win_init.patch (984 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] nsock/tests/tests_main.c

Gisle Vanem-2
"Henri Doreau" <[hidden email]> wrote:

> Why do they both initialize winsock with different version numbers?

Probably not important. But if one needs IPv6 functions from Winsock,
I imagined one must use MAKEWORD(2,2). Since addrset.c tests IPv6 (if
available), it seems to need 2.2. I cannot see that nsock/tests need IPv6.
Besides, I cannot confirm 2.2 with IPv6 since I run Win-XP without IPv6
installed.

> Does the simple patch attached sound good to you? I've stripped down
> the ANSI colors part. On trunk I simply print results without anything
> fancy when running on windows. They're not important and I'd like to
> keep it very simple. I'd actually rather use no colors at all than
> have complex platform-specific initialization code just for that.

Okay with me.

> I get double declaration errors which I need to investigate, it's
> probably nothing big. I find it ugly as hell but had no time to clean
> it since I noticed.

Many headers in Nmap are ugly; some are not atomic and step
on each other toes. Like you probably have seen.

--gv
_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] nsock/tests/tests_main.c

Henri Doreau
2013/8/12 Gisle Vanem <[hidden email]>:

> "Henri Doreau" <[hidden email]> wrote:
>
>> Why do they both initialize winsock with different version numbers?
>
>
> Probably not important. But if one needs IPv6 functions from Winsock,
> I imagined one must use MAKEWORD(2,2). Since addrset.c tests IPv6 (if
> available), it seems to need 2.2. I cannot see that nsock/tests need IPv6.
> Besides, I cannot confirm 2.2 with IPv6 since I run Win-XP without IPv6
> installed.
>
Thanks. I've changed it to 2.2 for nsock as well, to my understanding
this is what we want, even though there are no IPv6 tests yet.

>> Does the simple patch attached sound good to you? I've stripped down
>> the ANSI colors part. On trunk I simply print results without anything
>> fancy when running on windows. They're not important and I'd like to
>> keep it very simple. I'd actually rather use no colors at all than
>> have complex platform-specific initialization code just for that.
>
>
> Okay with me.
>
I've committed the ncat patch as r31789 and the nsock one as r31790.
Can you confirm if it works for you? Let me know if you find other
issues.

Regards

--
Henri
_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] nsock/tests/tests_main.c

Gisle Vanem-3
In reply to this post by Henri Doreau
"Henri Doreau" <[hidden email]> wrote:

> Thanks, fixed in r31734 (using a slightly different patch though).

Exactly 1 year has gone since the last message in this thread.
Now is the time for this function to be fixed. The win_init() function I
contributed was applied wrong:

--- orig/nsock/tests/tests_main.c        2014-06-25 10:48:30 +0000
+++ nsock/tests/tests_main.c    2014-06-25 11:49:42 +0000
@@ -81,8 +81,8 @@
 #ifdef WIN32
 static int win_init(void) {
   WSADATA data;
+  int  rc = WSAStartup(MAKEWORD(2, 2), &data);

-  rc = WSAStartup(MAKEWORD(2, 2), &data);
   if (rc)
     fatal("Failed to start winsock: %s\n", socket_strerror(rc));


--gv
_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] nsock/tests/tests_main.c

Henri Doreau
2014-08-10 14:23 GMT+02:00 Gisle Vanem <[hidden email]>:

> "Henri Doreau" <[hidden email]> wrote:
>
>> Thanks, fixed in r31734 (using a slightly different patch though).
>
>
> Exactly 1 year has gone since the last message in this thread. Now is the
> time for this function to be fixed. The win_init() function I contributed
> was applied wrong:
>
> --- orig/nsock/tests/tests_main.c        2014-06-25 10:48:30 +0000
> +++ nsock/tests/tests_main.c    2014-06-25 11:49:42 +0000
> @@ -81,8 +81,8 @@
> #ifdef WIN32
> static int win_init(void) {
>   WSADATA data;
> +  int  rc = WSAStartup(MAKEWORD(2, 2), &data);
>
> -  rc = WSAStartup(MAKEWORD(2, 2), &data);
>   if (rc)
>     fatal("Failed to start winsock: %s\n", socket_strerror(rc));
>
> --gv

My bad. Should be better now with r33462.

--
Henri
_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/