Samba 2.2.2 problem in lib/sprintf.c

[John E. Malmberg]

Andrew Tridgell wrote:
 > John Malmberg wrote:

>>The original code has proven to be broken on several platforms, and
>>will not build.  Either the compiler optimizer dies, or the resulting 
>>object modules will not link.
>>
> 
> the optimiser dies?!? That's a little hard to believe. Is the compiler
> on your system that broken? Link failures are also surprising.


That version of the compiler apparently had a bug, where it did not 
properly diagnose the condition where it was given conflicting instructions.


> 
> no, it's that many systems have broken snprintf() implementations. The
> C99 standard specifies some very specific behaviour that Samba now
> relies on (for very good reasons). Some systems don't follow that
> standard. We replace snprintf on those systems.


I understand that an existing snprintf() implementation may be broken.

One issue is that the configure test that Martin Pool posted is 
defective.  Without the prototypes, or compiler dependent flags, it is 
not really testing if the implementation has a broken snprintf.

What it is really testing is for an implementation that had a broken 
snprintf in the past, the compiler can still generate code to call that 
broken snprintf for applications that may be depend on the broken behavior.

The shortcut of skipping putting standard headers on a test program can 
very well invalidate it's results.

So until a fixed test is done, you may not know if the snprintf() is 
broken on a platform.


> 
>>Many systems will not allow a global procedure name conflict with one 
>>that is supplied in a standard library.  It produces a duplicate symbol 
>>conflict at link time.
>>
> 
> !?! then your linker is badly broken.


No, that is expected and well documented behavior when the standard 
library is in a shared image.

When you link against a shared image, you get all of the symbols.  So if 
you have named a global routine in a module the same as one in the 
library, that is a duplicate symbol violation.

This may not be signaled with all linkers.  But even on the systems 
where it is not signaled, depending on which symbol is used in some 
cases may be unreliable.


> Can you try the following simple
> program:
 
> #include <sys/types.h>
> 
> int snprintf(char *str, size_t size, const  char  *format, ...)
> {
> 	return 0;
> }
> 
> main()
> {
> 	return snprintf("foo", 0, 0);
> }
> 
> and see if it compiles. It should on any system. It won't give the
> same answer on all systems, but it should *always* compile.



EAGLE> cc tridge_snprintf.c/standard=ansi89

int snprintf(char *str, size_t size, const  char  *format, ...)
....^
%CC-I-INTRINSICDECL, In this statement, the declaration for intrinsic 
function snprintf" referenced at line number 10 in file 
PROJECT_ROOT:[SAMBA_VMS.SOURCE.TESTS]TRIDGE_SNPRINTF.C;1, the body of 
the function is being defined.  It will be treated as an ordinary 
external function.
at line number 3 in file 
PROJECT_ROOT:[SAMBA_VMS.SOURCE.TESTS]TRIDGE_SNPRINTF.C


Version T6.5 of the Compaq C compiler appears to have been fixed to 
diagnose the problem and report it correctly.

The code "-I-" in the diagnostic indicates that it is for 
"Informational" purposes only.

But when the snprintf() routine is in a separate module, and the 
standard header is used, then the intrinsic function will be used by the 
  compiler for the other modules.

So even though you have provided a replacement routine, none of the 
other modules will ever call it.  No diagnostic is required to inform 
the programmer of this.


>>If samba wants to supply it's own replacement routines for standard 
>>library calls, I recommend changing the name of the routines to have 
>>unique symbols like smb_snprintf() so that there is never a conflict 
>>with existing symbols in a standard library.
>>
> 
> we only do this when we absolutely have to. It is better to replace
> the broken function where possible.


I agree that a broken function needs to be replaced.  The issue is the 
way to do it that is portable, and will always produce the expected results.

The same is true when supplying missing routines.  A unique prefix 
should be applied.

Changing case to make a global symbol is allowed by the C standard, but 
is should be avoided.  While I can set the OpenVMS linking environment 
to be case sensitive on the global symbols, the default is to be case 
insensitive.

But the real reason to use prefixes like smb_ is that it is more obvious 
to a human that a different routine is desired.  A name with the wrong 
case does not stand out as well.

And if you decide to add in a third party library, a prefix is less 
likely to collide with any symbol creativity that they may have done.

Example: Realloc() in the UBIQX library caused a conflict SAMBA code 
written from before the UBIQX library was inserted.  This is documented 
in the comments in the source code.


> show me the compiler output from the above test program. Then maybe we
> can work out how to fix it. If we need to use a macro we will, but I
> would like to know why.


Because it is the only way that will produce reliable results on all 
platforms.



> I find it hard to believe that the above simple program won't

> compile on any system that is even vaguely POSIX.


Compile yes, but not without diagnostics.  If I drop back to 
/STANDARD=C89, the diagnostic does not appear.



I have asked before if there were specific coding standards that SAMBA 
is attempting to be compliant with.

There are several places where I have to override the compiler 
diagnostics because of issues where the SAMBA source code is not 
compliant with one of the C standards that also prevents it building on 
OpenVMS.

I usually only report the ones here that I know can affect other 
platforms than OpenVMS.

Normally I use the default settings, plus turning up the warning level 
to assist in finding programming errors.

Basically for this version of the compiler, it will diagnose violations 
of C99, and also will diagnose many questionable coding practices.


And inspite of what I am writting below, SAMBA for it's size is 
remarkably clean for GNU software in the quantity and quality of the 
diagnostics that are generated when I compile it.

It does not compile cleanly though.  Several of the diagnostic messages 
are from ANSI violations that the GCC compiler documents that it will 
allow with out diagnosis regardless of the warning level set.

But not everyone uses GCC.  And that does not cover all of the diagnostics.

 From reading this list for the past couple of years it is apparent that 
the SAMBA team does care about portability and having SAMBA compile clean.


But if you can name the standard that you are following, I will set my 
build environment to match, and then I can report the diagnostics that 
are flagged, usually with a patch that will eliminate it, as it is not 
worth my time to produce a report until I can get my build to complete 
with out diagnostics.


For example, with C90, global symbols must be unique within the first 31 
characters.  There are a few global symbols in SAMBA that fail that 
requirement.

This requirement may not be enforced on platforms other than OpenVMS, 
but it can also result in duplicate global symbols.

Since the other linkers also do not report duplicate global symbols, if 
they also have this limitation, it will only show up when the programs 
encounter the bad code at runtime.

Since I have not seen any reports of those type of bugs, I am assuming 
that of the platforms running SAMBA that OpenVMS is the only one that is 
affected.  Of course those are symbols like:

make_spoolss_q_getprinterdriver2
make_spoolss_q_getprinterdriverdir

If the wrong routine was called on some platform, it obviously would 
have been noticed by now, wouldn't it?


-John
wb8tyw at qsl.network
Personal Opinion Only