[PATCH] Array Out of Bounds in ratectrl_11n_ln.c

[biergaizi]

When I try to build the firmware with the latest GCC 4.9.0, it showed a warning for me:

[ 59%] Building C object CMakeFiles/firmware.dir/wlan/ratectrl_11n_ln.c.o
/data/Code/open-ath9k-htc-firmware/target_firmware/wlan/ratectrl_11n_ln.c: In function ‘rcSibUpdate_ht’:
/data/Code/open-ath9k-htc-firmware/target_firmware/wlan/ratectrl_11n_ln.c:373:42: warning: iteration 46u invokes undefined behavior [-Waggressive-loop-optimizations]
    mPhyCtrlState.validPhyRateIndex[i][j] = 0;
                                          ^
/data/Code/open-ath9k-htc-firmware/target_firmware/wlan/ratectrl_11n_ln.c:372:3: note: containing loop
   for (j = 0; j < MAX_TX_RATE_PHY; j++) {
   ^

This message means it must an undefined behavior in the piece of code

PHY_STATE_CTRL mPhyCtrlState;  

A_UINT8 i, j, k, hi = 0, htHi = 0;

for (i = 0; i < WLAN_RC_PHY_MAX; i++) {
    for (j = 0; j < MAX_TX_RATE_PHY; j++) {
        mPhyCtrlState.validPhyRateIndex[i][j] = 0;
    }   
    mPhyCtrlState.validPhyRateCount[i] = 0;
}

It looks fine. Right? But...

ratectrl.h

typedef struct phy_rate_ctrl {
        /* 11n state */
    A_UINT8  validPhyRateCount[WLAN_RC_PHY_MAX]; /* valid rate count */
    A_UINT8  validPhyRateIndex[WLAN_RC_PHY_MAX][MAX_TX_RATE_TBL]; /* index */    
}PHY_STATE_CTRL;

#ifdef MAGPIE_MERLIN  
#define MAX_TX_RATE_TBL         46
#define MAX_TX_RATE_PHY         48
#else
#define MAX_TX_RATE_TBL         54//46
#define MAX_TX_RATE_PHY         56//48
#endif

MAX_TX_RATE_TBL always greater than MAX_TX_RATE_PHY, the array out of bounds.

[biergaizi]

diff --git a/target_firmware/wlan/ratectrl_11n_ln.c b/target_firmware/wlan/ratectrl_11n_ln.c
index 277b184..de10a27 100755
--- a/target_firmware/wlan/ratectrl_11n_ln.c
+++ b/target_firmware/wlan/ratectrl_11n_ln.c
@@ -369,7 +369,7 @@ rcSibUpdate_ht(struct ath_softc_tgt *sc, struct ath_node_target *an,
        rcInitValidTxMask(pRc);

        for (i = 0; i < WLAN_RC_PHY_MAX; i++) {
-               for (j = 0; j < MAX_TX_RATE_PHY; j++) {
+               for (j = 0; j < MAX_TX_RATE_TBL; j++) {
                        mPhyCtrlState.validPhyRateIndex[i][j] = 0;
                }
                mPhyCtrlState.validPhyRateCount[i] = 0;

[olerem]

Good catch, thank you! Can you please completely remove MAX_TX_RATE_PHY and make a pull request.

[biergaizi]

Sure.

BTW, many people are talking about the newer GCC breaks their code, but in fact the code broken already. Many problems can be find by compiling the program with different compilers.

[biergaizi]

Please move to pull request #55.

[erikarn]

yup, the code is actually broken in a bunch of places. :)

-a

On 23 May 2014 08:32, Tom Li notifications@github.com wrote:

Sure.

BTW, many people are saying about the newer GCC breaks their code, but in fact the code broken already. Many problems can be find by compiling the program with different compilers.

— Reply to this email directly or view it on GitHubhttps://github.com/qca/open-ath9k-htc-firmware/issues/54#issuecomment-44026182 .

[erikarn]

.. and I'm totally happy with migrating to a newer GCC as long as there's been some significant testing by people to show that both the ar7010 and ar9271 firmware work using the newer GCC.

Thanks for kicking this along!

[biergaizi]

Thanks for merging :)

[olerem]

I'll start testing new GCC after merging my current patches

[erikarn]

Sweet!

I've also merged your other build changes too Oleksij. :-)

-a

On 23 May 2014 09:03, Oleksij Rempel notifications@github.com wrote:

I'll start testing new GCC after merging my current patches

— Reply to this email directly or view it on GitHubhttps://github.com/qca/open-ath9k-htc-firmware/issues/54#issuecomment-44029698 .

[olerem]

There is one more pull, i forgot to make a request.

[biergaizi]

I'm testing the latest toolchains by hacking Makefile. GMP_DIR is a little bit tricky...

@@ -1,25 +1,25 @@
-GMP_VER=5.0.5
+GMP_VER=6.0.0a
 GMP_URL=https://ftp.gnu.org/gnu/gmp/gmp-$(GMP_VER).tar.bz2
 GMP_TAR=gmp-$(GMP_VER).tar.bz2
-GMP_DIR=gmp-$(GMP_VER)
+GMP_DIR=gmp-6.0.0

-MPFR_VER=3.1.1
+MPFR_VER=3.1.2
 MPFR_URL=https://ftp.gnu.org/gnu/mpfr/mpfr-$(MPFR_VER).tar.bz2
 MPFR_TAR=mpfr-$(MPFR_VER).tar.bz2
 MPFR_DIR=mpfr-$(MPFR_VER)

-MPC_VER=1.0.1
+MPC_VER=1.0.2
 MPC_URL=https://ftp.gnu.org/gnu/mpc/mpc-$(MPC_VER).tar.gz
 MPC_TAR=mpc-$(MPC_VER).tar.gz
 MPC_DIR=mpc-$(MPC_VER)

-BINUTILS_VER=2.23.1
+BINUTILS_VER=2.24
 BINUTILS_URL=https://ftp.gnu.org/gnu/binutils/binutils-$(BINUTILS_VER).tar.bz2
 BINUTILS_TAR=binutils-$(BINUTILS_VER).tar.bz2
 BINUTILS_DIR=binutils-$(BINUTILS_VER)
 BINUTILS_PATCHES=local/patches/binutils.patch

-GCC_VER=4.7.2
+GCC_VER=4.9.0
 GCC_URL=https://ftp.gnu.org/gnu/gcc/gcc-$(GCC_VER)/gcc-$(GCC_VER).tar.bz2
 GCC_TAR=gcc-$(GCC_VER).tar.bz2
 GCC_DIR=gcc-$(GCC_VER)

[erikarn]

What about putting together a make environment variable that lets us say "new compiler" ?

-a

On 23 May 2014 09:12, Tom Li notifications@github.com wrote:

I'm testing the latest toolchains by hacking Makefile. GMP_DIR is a little bit tricky...

@@ -1,25 +1,25 @@ -GMP_VER=5.0.5 +GMP_VER=6.0.0a GMP_URL=https://ftp.gnu.org/gnu/gmp/gmp-$(GMP_VER).tar.bz2 GMP_TAR=gmp-$(GMP_VER).tar.bz2 -GMP_DIR=gmp-$(GMP_VER) +GMP_DIR=gmp-6.0.0

-MPFR_VER=3.1.1 +MPFR_VER=3.1.2 MPFR_URL=https://ftp.gnu.org/gnu/mpfr/mpfr-$(MPFR_VER).tar.bz2 MPFR_TAR=mpfr-$(MPFR_VER).tar.bz2 MPFR_DIR=mpfr-$(MPFR_VER)

-MPC_VER=1.0.1 +MPC_VER=1.0.2 MPC_URL=https://ftp.gnu.org/gnu/mpc/mpc-$(MPC_VER).tar.gz MPC_TAR=mpc-$(MPC_VER).tar.gz MPC_DIR=mpc-$(MPC_VER)

-BINUTILS_VER=2.23.1 +BINUTILS_VER=2.24 BINUTILS_URL=https://ftp.gnu.org/gnu/binutils/binutils-$(BINUTILS_VER).tar.bz2 BINUTILS_TAR=binutils-$(BINUTILS_VER).tar.bz2 BINUTILS_DIR=binutils-$(BINUTILS_VER) BINUTILS_PATCHES=local/patches/binutils.patch

-GCC_VER=4.7.2 +GCC_VER=4.9.0 GCC_URL=https://ftp.gnu.org/gnu/gcc/gcc-$(GCC_VER)/gcc-$(GCC_VER).tar.bz2 GCC_TAR=gcc-$(GCC_VER).tar.bz2 GCC_DIR=gcc-$(GCC_VER)

— Reply to this email directly or view it on GitHubhttps://github.com/qca/open-ath9k-htc-firmware/issues/54#issuecomment-44030656 .