RFR: 8329961: Buffer overflow in os::Linux::kernel_version [v3]

[Thomas Stuefe]

On Wed, 10 Apr 2024 09:14:36 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

> > Hmm, wouldn't sscanf not be simpler and safer? No need to factor out the parser. IMHO no need to even add a gtest since parsing would be really simple and not loop based. E.g.
> > ```
> > if (sscanf(release, "%d.%d", &major, &minor) != 2) {
> >   log_warning blabla
> > }
> > ```
> > 
> > 
> >     
> >       
> >     
> > 
> >       
> >     
> > 
> >     
> >   
> > As bonus, you avoid accidental conversion from hex numbers and such that strotol provides and that we don't really want here.
> 
> Hi, according to C11 standard (and my man pages) it is UB to call the scanf-family of functions with "invalid" data and `strtol` is recommended instead. So, unfortunately, it might not be safer.
> 

Can you point me to the relevant man page? If that were not to work, it would drastically limit the usefulness of scanf, and a lot of code in hotspot would be UB. 

My manpage:


The format string consists of a sequence of directives which describe how to process the sequence of input characters. If processing of a directive fails, no further input is read, and scanf() returns. A "failure" can be either of the following: input failure, meaning that input characters were unavailable, or matching failure, meaning that the input was inappropriate (see below). 


Matching failures are part of the API.

> Another thing: We shouldn't call `uname`. Reading `/proc/sys/kernel/osrelease` is sufficient. 

I agree with @robehn, uname is Posix and portable. May also be cached inside glibc, saving us a proc fs read.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/18697#issuecomment-2047304673