Skip to content

[Bug] remove use of variable length buffer#494

Merged
Mrs-X merged 1 commit intoPIVX-Project:masterfrom
rejectedpromise:bufferoverflowfix
Jan 20, 2018
Merged

[Bug] remove use of variable length buffer#494
Mrs-X merged 1 commit intoPIVX-Project:masterfrom
rejectedpromise:bufferoverflowfix

Conversation

@rejectedpromise
Copy link

@rejectedpromise rejectedpromise commented Jan 19, 2018

Remove use of variable length buffer from paymentservertests.cpp.
This removes the warning of:
stack protector not protecting local variables: variable length buffer
raised by the -Wstack-protector compilation flag.

As noted in issue: #387

@ghost ghost assigned rejectedpromise Jan 19, 2018
@ghost ghost added the review label Jan 19, 2018
@Fuzzbawls
Copy link
Collaborator

Fuzzbawls commented Jan 20, 2018

utACK d4e768b

AFAIK this was always more of a compile-time warning (not error) on some systems. Still needs solid testing and verification that nothing breaks during runtime.

Mrs-X
Mrs-X suggested changes Jan 20, 2018
View reviewed changes
Copy link

@Mrs-X Mrs-X left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK that is removes the compile-time warning, but NACK for the paymentserver-test itself, it fails now:

Before your change:

==========================================
   Pivx Core 3.0.99: src/test-suite.log
==========================================

# TOTAL: 3
# PASS:  3
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

After:

==========================================
   Pivx Core 3.0.99: src/test-suite.log
==========================================

# TOTAL: 3
# PASS:  2
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

The error itself:

********* Start testing of PaymentServerTests *********
Config: Using QtTest library 5.5.1, Qt 5.5.1 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 5.4.0 20160609)
PASS   : PaymentServerTests::initTestCase()
QDEBUG : PaymentServerTests::paymentServerTests() PaymentServer::initNetManager : No active proxy server found.
QDEBUG : PaymentServerTests::paymentServerTests() PaymentServer::processPaymentRequest : Secure payment request from  "testmerchant.org"
QWARN  : PaymentServerTests::paymentServerTests() PaymentRequestPlus::getMerchant : Payment request: certificate expired or not yet active:  QSslCertificate("3", "03", "LxHILx+N3qwVoAcCmQ5cyw==", (), ("Expired Test Merchant"), QMap(), QDateTime(2013-02-23 21:26:43.000 UTC Qt::TimeSpec(UTC)), QDateTime(2013-02-24 21:26:43.000 UTC Qt::TimeSpec(UTC)))
QDEBUG : PaymentServerTests::paymentServerTests() PaymentServer::processPaymentRequest : Insecure payment request to  "DJkvsVmdCTtpeTG1ChQuQ3Q1A8mMbKdAaL"
QWARN  : PaymentServerTests::paymentServerTests() PaymentRequestPlus::getMerchant : Payment request: certificate expired or not yet active:  QSslCertificate("3", "03", "LxHILx+N3qwVoAcCmQ5cyw==", (), ("Expired Test Merchant"), QMap(), QDateTime(2013-02-23 21:26:43.000 UTC Qt::TimeSpec(UTC)), QDateTime(2013-02-24 21:26:43.000 UTC Qt::TimeSpec(UTC)))
QDEBUG : PaymentServerTests::paymentServerTests() PaymentServer::processPaymentRequest : Secure payment request from  "testmerchant8.org"
QWARN  : PaymentServerTests::paymentServerTests() PaymentRequestPlus::getMerchant : Payment request: certificate expired or not yet active:  QSslCertificate("3", "06", "MiZaQ+g9lSHZGuHWkXZG+g==", (), ("Payment Request Intermediate 5"), QMap(), QDateTime(2013-02-23 22:59:51.000 UTC Qt::TimeSpec(UTC)), QDateTime(2013-02-24 22:59:51.000 UTC Qt::TimeSpec(UTC)))
QDEBUG : PaymentServerTests::paymentServerTests() PaymentServer::processPaymentRequest : Insecure payment request to  "DJkvsVmdCTtpeTG1ChQuQ3Q1A8mMbKdAaL"
QWARN  : PaymentServerTests::paymentServerTests() PaymentRequestPlus::getMerchant : Payment request: certificate expired or not yet active:  QSslCertificate("3", "06", "MiZaQ+g9lSHZGuHWkXZG+g==", (), ("Payment Request Intermediate 5"), QMap(), QDateTime(2013-02-23 22:59:51.000 UTC Qt::TimeSpec(UTC)), QDateTime(2013-02-24 22:59:51.000 UTC Qt::TimeSpec(UTC)))
QWARN  : PaymentServerTests::paymentServerTests() PaymentRequestPlus::getMerchant : SSL error:  certificate signature failure
QDEBUG : PaymentServerTests::paymentServerTests() PaymentServer::processPaymentRequest : Insecure payment request to  "DJkvsVmdCTtpeTG1ChQuQ3Q1A8mMbKdAaL"
QWARN  : PaymentServerTests::paymentServerTests() PaymentRequestPlus::getMerchant : SSL error:  certificate signature failure
QWARN  : PaymentServerTests::paymentServerTests() PaymentRequestPlus::getMerchant : SSL error:  unable to get local issuer certificate
QDEBUG : PaymentServerTests::paymentServerTests() PaymentServer::processPaymentRequest : Insecure payment request to  "DJkvsVmdCTtpeTG1ChQuQ3Q1A8mMbKdAaL"
QWARN  : PaymentServerTests::paymentServerTests() PaymentRequestPlus::getMerchant : SSL error:  unable to get local issuer certificate
QFATAL : PaymentServerTests::paymentServerTests() Received signal 11
FAIL!  : PaymentServerTests::paymentServerTests() Received a fatal error.
   Loc: [Unknown file(0)]
Totals: 1 passed, 1 failed, 0 skipped, 0 blacklisted
********* Finished testing of PaymentServerTests *********

@rejectedpromise
Copy link
Author

rejectedpromise commented Jan 20, 2018

@Mrs-X please run again and let me know if test now passes.
@Fuzzbawls, that is correct, as noted in the description

@Mrs-X
Copy link

Mrs-X commented Jan 20, 2018

Null-terminating is always a good idea :-)

ACK and merging...

@Mrs-X Mrs-X merged commit d0e6e6b into PIVX-Project:master Jan 20, 2018
Mrs-X added a commit that referenced this pull request Jan 20, 2018
@Mrs-X
Merge #494: [Bug] remove use of variable length buffer
8180b78 
d0e6e6b fix paymentservertests.cpp to use automatic buffer overflow protection (rejectedpromise)

Tree-SHA512: b6cc56afb452544a0b20ebe21f1464ab906c786221fcc779f77f241ef26f0d61a7d86c859060daf8e958af6c6ed7a9887542f4719dfeaaf03733eb39e45fb14d
@ghost ghost removed the review label Jan 20, 2018
@rejectedpromise
Copy link
Author

rejectedpromise commented Jan 20, 2018

I think I was being a big dummy last night, probably tired too ;), and running test_pivx not test_pivxqt 🤦‍♂️

@Fuzzbawls Fuzzbawls added this to the 3.1.0 milestone Jan 20, 2018
@rejectedpromise rejectedpromise deleted the bufferoverflowfix branch June 9, 2018 22:20
@random-zebra random-zebra mentioned this pull request Feb 5, 2021
Merged
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Feb 13, 2021
@random-zebra
[BUG] Fix sizeof in paymentservertests
613e1da 
Partially reverting PIVX-Project#494
Fuzzbawls added a commit that referenced this pull request Feb 14, 2021
@Fuzzbawls
Merge #2189: [Bug][Refactoring] Miscellaneous fixes
71ed429 
84b4185 [Tests] raise minimum fee in feature_blockindexstats (random-zebra)
39d8a20 [Cleanup] Remove OldSetKeyFromPassphrase/OldEncrypt/OldDecrypt (random-zebra)
613e1da [BUG] Fix sizeof in paymentservertests (random-zebra)
0f3e961 [Cleanup] Remove useless call (random-zebra)
b1ca5e0 Remove unused fsbridge::freopen (practicalswift)
b3a1d84 Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)
ecfbcfd [Cleanup] Remove unused function AddInvalidSpendsToMap (random-zebra)
15b018c [Refactor] Use InsecureRand in the unit tests (random-zebra)
81fd84c [Refactoring] Replace risky call to rand() with GetRandInt() (random-zebra)
8ba1978 [Qt] Terminate string *pszExePath after readlink and without using memset (practicalswift)
b6cd719 Remove unreachable code (g_rpcSignals.PostCommand) (practicalswift)
e4f9ab3 [BUG] Memory leak after new CNode / ConnectNode (random-zebra)
536122b Avoid rollingMinimumFeeRate never being able to decay below half (Alex Morcos)
13cad19 fix a bug if the min fee is 0 for FeeFilterRounder (Alex Morcos)
aa832d8 [Refactoring] Dereference before/after null check (random-zebra)
4630b7d [Trivial] Pass big parameters by reference, not value (random-zebra)
aa7bc7f [Refactoring] Remove unneeded CScript::IsNormalPaymentScript (random-zebra)
6cf3c8f [Trivial] Unitialized scalar fields and variables (random-zebra)

Pull request description:

  This is a collection of small fixes for the following issues/defects (discovered with the coverity tool):

  - Big parameters passed by value
  - calls to rand() function
  - Dereference before/after null-pointer check
  - Pointer to local variable out of scope
  - Resource leak with call to ConnectNode
  - Non-floating point result - unintended integer division (bitcoin#9288)
  - String not-null-terminated (bitcoin#11193)
  - Structurally dead code (bitcoin#9575)
  - Unchecked boolean return value of functions (GetOp, GetTransaction, TxOutToPublicCoin)
  - Unitialized pointers and scalar fields
  - Illegal memory access (bitcoin#13159)
  - Useless call to pubkey.IsCompressed()
  - Wrong `sizeof` argument (bitcoin#11024 + revert #494)

ACKs for top commit:
  furszy:
    Looking good, ACK 84b4185
  Fuzzbawls:
    ACK 84b4185

Tree-SHA512: 4c920a1858ccde65795c090e4becc47c50c0a78db7ab11935b4d3e2bea3f7f1f8ca3b48ce633d8112673092c35ae7cd05c444ed2b16e283a305c765874e55c1c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Mrs-X Mrs-X Mrs-X approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rejectedpromise rejectedpromise

Labels

Bug

Projects

None yet

Milestone

3.1.0

Development

Successfully merging this pull request may close these issues.

3 participants

@rejectedpromise @Fuzzbawls @Mrs-X