Page MenuHome GnuPG

dn.cpp:181: suspicious loop
Closed, ResolvedPublic

Description

dn.cpp:181:13: warning: variable 's' is incremented both in the loop header and in the loop body [-Wfor-loop-analysis]

Source code is

for (s = string; hexdigitp(s); s++) {
     s++;
 }

It is strange to only check every other digit. Suggest add comment or rework the code.

Related Objects

Event Timeline

We have the same flaw in gnupg.

Fixed in gnupg and gpgme. it is not serious because that is just a failsafe check; libksba creates these strings and it does it correctly.

Thanks for the report.

I found the bug by compiling the package with C/C++ compiler clang and flag -Wall.

The project might find it useful to also do this compilation and have a look
at the warnings produced.

I found over 1,000 warnings. List available on request.

-Wall is not a good idea in general because it is too unspecific. This is why we have a list of useful warning and warnings we ignore with gcc.

Thanks for looking at the warnings and reporting this case. This for check seems to be new and as soon as it shows up in gcc we can test for its availability and use it. We still need to replace our old Jenkins installation by buildbot and after we have done that we can add clang specific options to the projects.

FWIW: A particular problematic thing are warning pertaining the use of unitialized variables. They show up and go depending on the compiler versions. Some are really important but others are not correct because a variable'ss use is protected by another variable. Simply initializing all those variables has the drawback that it makes it impossible to detect a really forgotten initalization. So this needs to be decided case by case.

-Wall is not a good idea in general because it is too unspecific. This is why we have a list of useful warning and >warnings we ignore with gcc.

Thousands of projects are quite happy with gcc compiler flags -g -O2 -Wall.
Ok not everything gcc finds is worth fixing, but in general warnings are IMHO very much worth
looking at.

gcc isn't a perfect tool. It has strengths and weaknesses. But in general, warnings
produced by -Wall are well worth looking deeper at. gcc developers designed it that way.

Thanks for looking at the warnings and reporting this case. This for check seems to be new and as soon as it shows >up in gcc we can test for its availability and use it.

The for check isn't in gcc, it is specific to clang.

FWIW: A particular problematic thing are warning pertaining the use of unitialized variables.

Certainly the gcc implementation to find uninit variables isn't perfect, but it is generally useful.

I am not familiar with your project and so it may well have special needs, but in general, I would be deeply
suspicious of any project that chooses to generally ignore warnings produced by -Wall.

FWIW, here an example of warnings we use. Yes it starts with -Wall but there are a couple of more specific warnings and at a few places we even use pragmas to disable warnings. And it depends on the compiler version used.

-Wall -Wcast-align -Wshadow -Wstrict-prototypes
-Wformat -Wno-format-y2k -Wformat-security -W -Wno-sign-compare
-Wno-missing-field-initializers -Wdeclaration-after-statement
-Wlogical-op -Wvla -Wno-pointer-sign -Wpointer-arith