- Download Diff

Summary:

9169 libsldap: comparison between pointer and zero character constant

Review Request #896 — Created Feb. 22, 2018 and submitted Sept. 30, 2018, 7:15 p.m.

Information
Owner:	tsoome
Repository:	illumos-gate
Branch:
Bugs:	9169
Depends On:
Commit:	6ba0443...
Reviewers
Groups:	general
People:

Description

../common/ns_crypt.c:101:19: error: comparison between pointer and zero character constant [-Werror=pointer-compare]
   if (++anHexaStr != '\0') {
                   ^~
../common/ns_crypt.c:101:7: note: did you mean to dereference the pointer?
   if (++anHexaStr != '\0') {
       ^~

../common/ns_writes.c: In function '__s_cvt_hosts':
../common/ns_writes.c:2035:54: error: comparison between pointer and zero character constant [-Werror=pointer-compare]
      ptr->h_addr_list == NULL || ptr->h_addr_list[0] == '\0') {
                                                      ^~
../common/ns_writes.c:2035:34: note: did you mean to dereference the pointer?
      ptr->h_addr_list == NULL || ptr->h_addr_list[0] == '\0') {
                                  ^
../common/ns_writes.c: In function '__s_cvt_services':
../common/ns_writes.c:2347:61: error: comparison between pointer and zero character constant [-Werror=pointer-compare]
  if (ptr->s_name == NULL || ptr->s_port < 0 || ptr->s_proto == '\0') {
                                                             ^~
../common/ns_writes.c:2347:48: note: did you mean to dereference the pointer?
  if (ptr->s_name == NULL || ptr->s_port < 0 || ptr->s_proto == '\0') {
                                                ^
../common/ns_writes.c: In function '__s_cvt_netmasks':
../common/ns_writes.c:2561:19: error: comparison between pointer and zero character constant [-Werror=pointer-compare]
  if (ptr->netmask != '\0') {
                   ^~
../common/ns_writes.c:2561:6: note: did you mean to dereference the pointer?
  if (ptr->netmask != '\0') {
      ^
../common/ns_writes.c: In function '__s_cvt_netgroups':
../common/ns_writes.c:2619:16: error: comparison between pointer and zero character constant [-Werror=pointer-compare]
  if (ptr->name != '\0') {
                ^~
../common/ns_writes.c:2619:6: note: did you mean to dereference the pointer?
  if (ptr->name != '\0') {
      ^
../common/ns_writes.c: In function '__s_cvt_bootparams':
../common/ns_writes.c:2718:16: error: comparison between pointer and zero character constant [-Werror=pointer-compare]
  if (ptr->name != '\0') {
                ^~
../common/ns_writes.c:2718:6: note: did you mean to dereference the pointer?
  if (ptr->name != '\0') {
      ^
../common/ns_writes.c: In function '__s_cvt_ethers':
../common/ns_writes.c:2782:38: error: comparison between pointer and zero character constant [-Werror=pointer-compare]
  if (ptr->name == NULL || ptr->ether == '\0') {
                                      ^~
../common/ns_writes.c:2782:27: note: did you mean to dereference the pointer?
  if (ptr->name == NULL || ptr->ether == '\0') {
                           ^
cc1: all warnings being treated as errors

Testing Done

Issues

0
0
0
1
All Issues: 1

Description	From	Last Updated
I guess here was missing dereference, but you imporoved it and move this check to the line 3016 (after). Do ...	vgusev	July 25, 2018, 9:03 a.m.

Ship it!

That's really weird code, but as the logic didn't change thanks to '\0' and NULL being the same, ship it.

Status: Closed (submitted)

Status: Re-opened

Change Summary:

ok there is some confusion about what has to be checked and protected.

So, first of all, we perform updates by calling __s_add_attr() while we provide strings as attribute name and value. s_add_attr() does call strdup() which will call strlen() which will crash us if NULL pointer is passed — therefore it is essential to prevent NULL pointers being passed.

Now the empty strings are a bit different story - we may want to create (attribute, value) pairs with empty string as a value - for example, when we want to make sure the attribute itself is there, even if not populated.

We also can not have empty names, or we will get empty value for “cn” attribute, and this can not happen.

Commit:

-	f5a6001cfc851404c1b04f539ded4b0844758576
+	5bdd2249a16d1bfbe162024e3840302dddb8fdfd

Diff:

Revision 2 (+34 -40)

Show changes

	usr/src/lib/libsldap/common/ns_crypt.c
	usr/src/lib/libsldap/common/ns_writes.c

Change Summary:

cstyle cleanup.

Commit:

-	5bdd2249a16d1bfbe162024e3840302dddb8fdfd
+	74b139b4a3ec651649cbbf13a3513d29c861a78c

Diff:

Revision 3 (+74 -80)

Show changes

	usr/src/lib/libsldap/common/ns_crypt.c
	usr/src/lib/libsldap/common/ns_writes.c

usr/src/lib/libsldap/common/ns_writes.c (Diff revision 3)

Seeing all these needless indentation changes in reviews (wasting my reviewer time) still annoys me...

There is absolutely nothing wrong with a tab here.

Grumble!

tsoome March 12, 2018, 4:49 a.m.

If there is nothing wrong, then pbchk shouldn't complain.

gwr March 12, 2018, 3:56 p.m.

I tried. That was https://www.illumos.org/issues/8419
This is an example of the cost of that bug.
This review is now larger than what I have time for.

yuripv March 12, 2018, 4:05 p.m.

And you did agree that it's only you who wants that, so why bring it up again? :-)

yuripv March 12, 2018, 4:14 p.m.

BTW, if you select diff 0-2 (i.e. not including last change), you'll get a review without cstyle changes that you don't like.

Ship it!

```
Ship It!
```

Change Summary:

missing ||

Commit:

-	74b139b4a3ec651649cbbf13a3513d29c861a78c
+	e58a28912b4dbf729517844b8faa6f020e9f04cd

Diff:

Revision 4 (+74 -80)

Show changes

	usr/src/lib/libsldap/common/ns_crypt.c
	usr/src/lib/libsldap/common/ns_writes.c

Ship it!

```
Ship It!
```

Change Summary:

few white space nits

Commit:

-	e58a28912b4dbf729517844b8faa6f020e9f04cd
+	6ba04432ee1a75ea0218969d11a0898586b05e10

Diff:

Revision 5 (+83 -89)

Show changes

	usr/src/lib/libsldap/common/ns_crypt.c
	usr/src/lib/libsldap/common/ns_writes.c

usr/src/lib/libsldap/common/ns_writes.c (Diff revision 5)

Show all issues

I guess here was missing dereference, but you imporoved it and move this check to the line 3016 (after).
Do you think it was by design to add non empty alias ? I am asking because before-patch it is allowed, but after patch behaviour is changed.

If you are sure, feel free drop Issue.

tsoome July 25, 2018, 9:07 a.m.

The function is to convert aliases, and to do that, they first create rdn at line #3026 (#3023 in update), if the ptr->alias is empty string, we will get rdn "cn=" and that should alarm anyone who has worked with LDAP -- its clearly a problem. It also does mean, we do not have alias to be converted, and we can return with error. I think it actually is "impossible" error, but we should expect unexpected there:)
Note, the other tables do have similar checks in place, although I'm not sure if all those other cases are handled correctly, but thats another issue anyhow.

You have a pending review.

Review Board 3.0.18

9169 libsldap: comparison between pointer and zero character constant

Status: Closed (submitted)

Status: Re-opened

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Status: Closed (submitted)