Bug 2026563

Created attachment 1843583 [details]
Proposed fix

This is nice old school bug :) Attached patch my be not the optimal fix, probably also better to increase the buffer a bit. I will forward it to upstream.

Created attachment 1843588 [details]
Proposed fix

Simplified version.

Upstream ticket:
https://sourceforge.net/p/acpitool/patches/2/

Hmm,

echo "5.14.18-300.fc35.x86_64" | wc -c
24

int Do_SysVersion_Info(int verbose)
    char Acpi_Version[10], Kernel_Version[15];

    memset(Acpi_Version, '\0', 10);
    memset(Kernel_Version, '\0', 15);

...

    file_in.getline(str, 16);
    file_in.close();

    strcpy(c, str);


your patch:

-    file_in.getline(str, 16);
+    file_in.getline(str, 15);

Hopefully, the 15th char is always \0

Beside cutting kernel version imho the code need some more review, e.g. increasing buffer sizes, replacing strcpy by strncpy (or snprintf) and feed strlen of strings through functions.

(In reply to Peter Bieringer from comment #4)
> Hmm,
> 
> echo "5.14.18-300.fc35.x86_64" | wc -c
> 24
> 
> int Do_SysVersion_Info(int verbose)
>     char Acpi_Version[10], Kernel_Version[15];
> 
>     memset(Acpi_Version, '\0', 10);
>     memset(Kernel_Version, '\0', 15);
> 
> ...
> 
>     file_in.getline(str, 16);
>     file_in.close();
> 
>     strcpy(c, str);
> 
> 
> your patch:
> 
> -    file_in.getline(str, 16);
> +    file_in.getline(str, 15);
> 
> Hopefully, the 15th char is always \0
> 
Nope, the getline adds the explicit NULL, see the docs, that's why I removed the explicit zeroing in the second version of the patch:

>> In any case, if count>0, it then stores a null character CharT() into the next successive location of the array and updates gcount(). 

https://en.cppreference.com/w/cpp/io/basic_istream/getline


> Beside cutting kernel version imho the code need some more review, e.g.
> increasing buffer sizes, replacing strcpy by strncpy (or snprintf) and feed
> strlen of strings through functions.

I agreed it may require increase of the buffer, I mentioned it in the comments and upstream.

Hi,
I ran into the same issue. I agree that fixing upsteam is usually the best way to go, but upstream seems abandoned (see e.g. Fedora patches adding Linux 3.x compatibility or the comment on Sourceforge: "Last Update: 2016-03-14").

Therefore I propose to directly fix the package as a patch to Fedora. See uploaded patch file. I used a different approach than in the previous comments: As the original acpitool.cpp file already includes the <string> header, I use std::string for the buggy code, trying to keep formatting as close to the origin as possible.

Does anyone have any objections if I try to cleanup the ~80 warnings in a separate patch? Whitespacing is random as well...

Created attachment 1873729 [details]
Replace char by std::string to avoid buffer overflows for long headers

Avoids the buffer overflow with '-e' option and long kernel versions.

This message is a reminder that Fedora Linux 35 is nearing its end of life.
Fedora will stop maintaining and issuing updates for Fedora Linux 35 on 2022-12-13.
It is Fedora's policy to close all bug reports from releases that are no longer
maintained. At that time this bug will be closed as EOL if it remains open with a
'version' of '35'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, change the 'version' 
to a later Fedora Linux version.

Thank you for reporting this issue and we are sorry that we were not 
able to fix it before Fedora Linux 35 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora Linux, you are encouraged to change the 'version' to a later version
prior to this bug being closed.

acpitool-0.5.1-29.fc37.x86_64 is still affected

This message is a reminder that Fedora Linux 37 is nearing its end of life.
Fedora will stop maintaining and issuing updates for Fedora Linux 37 on 2023-12-05.
It is Fedora's policy to close all bug reports from releases that are no longer
maintained. At that time this bug will be closed as EOL if it remains open with a
'version' of '37'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, change the 'version' 
to a later Fedora Linux version. Note that the version field may be hidden.
Click the "Show advanced fields" button if you do not see it.

Thank you for reporting this issue and we are sorry that we were not 
able to fix it before Fedora Linux 37 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora Linux, you are encouraged to change the 'version' to a later version
prior to this bug being closed.

Issue still exists on F39:

rpm -qf $(which acpitool)
acpitool-0.5.1-31.fc39.x86_64

acpitool -e -v
*** buffer overflow detected ***: terminated
Aborted (core dumped)

This message is a reminder that Fedora Linux 39 is nearing its end of life.
Fedora will stop maintaining and issuing updates for Fedora Linux 39 on 2024-11-26.
It is Fedora's policy to close all bug reports from releases that are no longer
maintained. At that time this bug will be closed as EOL if it remains open with a
'version' of '39'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, change the 'version' 
to a later Fedora Linux version. Note that the version field may be hidden.
Click the "Show advanced fields" button if you do not see it.

Thank you for reporting this issue and we are sorry that we were not 
able to fix it before Fedora Linux 39 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora Linux, you are encouraged to change the 'version' to a later version
prior to this bug being closed.

Problem still existing in Fedora 41

This package has changed maintainer in Fedora. Reassigning to the new maintainer of this component.

This package has changed maintainer in Fedora. Reassigning to the new maintainer of this component.