NOTE: Please always add static_assert on datatype before using FmtCore::format

@jinzx10

Describe the Code Quality Issue

After a very long time debugging, @jinzx10, @caic99 and I find the bug reported by issue #4540.
See the following code piece:

abacus-develop/source/module_cell/klist.cpp

Line 776 in db90f92

std::vector<double> ibz2bz(this->nkstot);

what will happen if:

abacus-develop/source/module_cell/klist.cpp

Lines 944 to 953 in db90f92

    
           for (int ik = 0; ik < nkstot_ibz; ik++) 
        
           { 
        
               table += FmtCore::format("%8d%12.8f%12.8f%12.8f%8.4f%8d\n", 
        
                                        ik + 1, 
        
                                        kvec_d_ibz[ik].x, 
        
                                        kvec_d_ibz[ik].y, 
        
                                        kvec_d_ibz[ik].z, 
        
                                        wk_ibz[ik], 
        
                                        ibz2bz[ik]); 
        
           }

?
It will trigger an undefined behavior because the double is parsed like int by formatter (the ABACUS in-built library for formatting strings, implemented in module_base/formatter.h), that cases recent failure in integrated test.

The implementation of function FmtCore::format is quite simple:

abacus-develop/source/module_base/formatter.h

Lines 40 to 48 in db90f92

    
           template<typename... Ts> 
        
           static inline std::string format(const char* fmt, const Ts&... args) 
        
           { 
        
               const int size = snprintf(nullptr, 0, fmt, FmtCore::filter(args)...) + 1; 
        
               std::string dst(size, ' '); 
        
               const int size_filled = snprintf(&dst[0], size, fmt, FmtCore::filter(args)...); 
        
               dst.resize(size_filled); 
        
               return dst; 
        
           }

But the c-style function snprintf itself cannot identify the datatype. So I would suggest to use something like static_assert before using this function.

Additional Context

No response

Task list for Issue attackers (only for developers)

Identify the specific code file or section with the code quality issue.
Investigate the issue and determine the root cause.
Research best practices and potential solutions for the identified issue.
Refactor the code to improve code quality, following the suggested solution.
Ensure the refactored code adheres to the project's coding standards.
Test the refactored code to ensure it functions as expected.
Update any relevant documentation, if necessary.
Submit a pull request with the refactored code and a description of the changes made.

@WHUweiqingzhou

@WHUweiqingzhou FYI

Most compilers provide a "-Wformat" option for basic type checks for printf/sprintf/..., which would emit a warning message when there is a type mismatch. For example, compiling the following code

#include <cstdio>

int main() {
    double d = 3;
    std::snprintf(nullptr, 0, "%4i", d);
    return 0;
}

will get something like

hw.cpp: In function ‘int main()’:
hw.cpp:41:34: warning: format ‘%i’ expects argument of type ‘int’, but argument 4 has type ‘double’ [-Wformat=]
   41 |     std::snprintf(nullptr, 0, "%4i", d);
      |                                ~~^   ~
      |                                  |   |
      |                                  int double
      |                                %4f

I'm not sure if "-Wformat" is enabled by default [at least it is so on my everyday compiler gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04)], but even it is enabled, the real question is, "-Wformat" is merely a compile-time check that is unable to be carried out for argument types agains a format string passed via function argument, as it is in FmtCore. That is to say, "-Wformat" would fail to detect bugs like

#include <cstdio>

void foobar(const char* fmt, double i) {
    std::snprintf(nullptr, 0, fmt, i);
}

int main() {
    foobar("%4i", 3.0);
    return 0;
}

It might, however, be possible to let snprintf knows the format string during compilation, as long as the format string is a const expression (e.g., making "foobar" a function template and fmt a template parameter). Unfortunately, using string literals as template parameters is a c++20 feature; achieving this within c++11, if possible, might need some metaprogramming.

@jinzx10

@jinzx10 IMHO using macro definition would be a good replacement

#include <cstdio>

#define foobar_safe(fmt, i) std::snprintf(nullptr, 0, fmt, i)

int main() {
    foobar_safe("%4i", 3.0);
}

This grammar generates warning correctly.

@caic99

@caic99
I will use the following version:

#include <cstdio>

#define __FMTCORE_DATA_CHECK__(fmt, ...) std::snprintf(nullptr, 0, fmt, __VA_ARGS__)

int main() {
    const int right = 3;
    const double wrong = 3.0;

    __FMTCORE_DATA_CHECK__("%4d%4d", right, wrong);
}

@kirk0830

I think in the beginning of the issue, there should be a link to explain what is 'formatter' @kirk0830

@kirk0830

I think in the beginning of the issue, there should be a link to explain what is 'formatter' @kirk0830

Thanks, I have added a short explanation about formatter in issue descrption

I move this issue to discussion Note for Developer.

Navigation Menu

Repository navigation

NOTE: Please always add `static_assert` on datatype before using `FmtCore::format` #4576

Describe the Code Quality Issue

Additional Context

Task list for Issue attackers (only for developers)

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	for (int ik = 0; ik < nkstot_ibz; ik++)
	{
	table += FmtCore::format("%8d%12.8f%12.8f%12.8f%8.4f%8d\n",
	ik + 1,
	kvec_d_ibz[ik].x,
	kvec_d_ibz[ik].y,
	kvec_d_ibz[ik].z,
	wk_ibz[ik],
	ibz2bz[ik]);
	}

	template<typename... Ts>
	static inline std::string format(const char* fmt, const Ts&... args)
	{
	const int size = snprintf(nullptr, 0, fmt, FmtCore::filter(args)...) + 1;
	std::string dst(size, ' ');
	const int size_filled = snprintf(&dst[0], size, fmt, FmtCore::filter(args)...);
	dst.resize(size_filled);
	return dst;
	}

NOTE: Please always add static_assert on datatype before using FmtCore::format #4576

Description

Describe the Code Quality Issue

Additional Context

Task list for Issue attackers (only for developers)

Activity

kirk0830 commented on Jul 4, 2024

jinzx10 commented on Jul 4, 2024

caic99 commented on Jul 5, 2024

kirk0830 commented on Jul 5, 2024

mohanchen commented on Jul 8, 2024

kirk0830 commented on Jul 8, 2024

WHUweiqingzhou commented on Jul 11, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

NOTE: Please always add `static_assert` on datatype before using `FmtCore::format` #4576