trafficserver icon indicating copy to clipboard operation
trafficserver copied to clipboard

Proposition for removing the global suppression `-Wno-stringop-overflow` from the build

Open freak82 opened this issue 1 year ago • 1 comments

Hi there,

As far as I checked the only place where this warning is raised is here:

/z/x3me-ocn/src/iocore/net/OCSPStapling.cc:1166:12: warning: ‘char* strncat(char*, const char*, size_t)’ specified bound 1 equals source length [-Wstringop-overflow=]
 1166 |     strncat(url->end(), "/", 1);

Here is the code in question:

  if (url->buf()[url->size() - 1] != '/') {                                     
    strncat(url->end(), "/", 1);                                                
    url->fill(1);                                                                   
  } 

Note that the url buffer size is calculated above in such a way that there is a guarantee that it's big enough for this concatenation.

This type of GCC warning is a bit bogus. However, I think with a slight change the code here can be made better/safer and at the same time the global suppression of this warning can be removed. Although it's not very obvious (IMO) from the man page of strncat, the last size_t argument is supposed to specify the remaining space in the destination buffer. So, I think this code is better written as:

  if (url->buf()[url->size() - 1] != '/') {                                     
    strncat(url->end(), "/", url->write_avail());                                                
    url->fill(1);                                                                   
  } 

or

 if (url->buf()[url->size() - 1] != '/') {                                     
      written = ink_strlcat(url->end(), "/", url->write_avail());          
      url->fill(written);                                                                
  } 

What do you think?

freak82 avatar May 13 '24 10:05 freak82

The proposed code looks good, and I'm +1 on removing the suppression. I'd go with ink_strlcat because use of strncat isn't common in ATS codebase. Would you make a Pull Request?

maskit avatar May 13 '24 19:05 maskit

I'm OK to do a pull request but I'm not sure against which branch to do it?

freak82 avatar May 14 '24 05:05 freak82

A Pull Request against master would be great. We'll take care of other branches.

maskit avatar May 14 '24 15:05 maskit