[c++] Add bounds check in StringToArrayFast() to prevent heap buffer over-read

[sanjay20m]

Summary

This PR fixes a memory safety issue in StringToArrayFast() where the parser
could read past the end of the model string when the declared array size was
larger than the available data.

Vulnerability

An attacker could create a malicious LightGBM text model file with:

• A large declared array size
• Fewer actual numeric values

This mismatch would cause the parser to read beyond the allocated buffer,
triggering undefined behavior. This can result in:

• Denial of Service (process crash)
• Possible information disclosure

Fix

• Added a check to ensure the parser does not read beyond the string's end.
• Logs a fatal error and aborts safely if the file is malformed.

Security Impact

This hardens the model loading path against malicious or corrupted model files.
The patch does not change public APIs or intended parsing behavior.

---

[jameslamb]

Thanks for your interest in LightGBM.

How did you discover this? How can we test it? Can you share a normal model file and one modified in the way you say this protects against, so we can understand what's being proposed here?

[sanjay20m]

Hi @jameslamb
I discovered this while inspecting the StringToArrayFast() implementation and noticed that if the declared array size (n) is larger than the number of values in the input string, the parser will keep advancing the pointer past the end of the string buffer. This could happen if a model file is corrupted or manually edited.

How to reproduce

1. Train any LightGBM model (e.g., using examples/regression) and save it as a text model file.
2. Open the file and locate a numeric array line, such as:
   thresholds=0.1 0.5 0.9
3. Edit the corresponding metadata or header so that the declared array size is larger than the number of values in that array (for example, 10 instead of 3).
4. Load the modified model:

• Before this patch → parser reads beyond the buffer, which may cause a crash or unexpected values.
• With this patch → parser stops and logs:

  Fatal: Malformed model file: not enough values in string for array of size X
  

This demonstrates that the change prevents a possible heap buffer over-read when loading malformed model files.

[sanjay20m]

Hello @jameslamb ,

Can u take some time to review!
thanks!!

[sanjay20m]

Hi @jameslamb

Following up on this! To make testing this easier, I have attached a .zip file containing two minimal model files:

1. normal_model.txt (a standard, unmodified model)
2. malformed_model.txt (the same model, but with the array size artificially inflated to demonstrate the heap buffer over-read)

If you load malformed_model.txt on the current master branch, you will see the parser read beyond the buffer. With this PR applied, it safely catches the error and logs: Fatal: Malformed model file: not enough values in string for array of size X.

Could you take another look when you have a moment? Let me know if you need any other details!
model_repro_files.zip

[jameslamb]

Thank you for your continued interest.

Sorry but I'm not opening a .zip from someone I don't know, that's a security risk.

You will have to be patient and wait for someone to review this.

[sanjay20m]

Hey @jameslamb

Fair point on the .zip file, my bad.

If you or anyone on the team wants to verify this locally without downloading an archive, here is the minimal script to generate the 1-tree model:

import lightgbm as lgb
import numpy as np
X = np.array([[1.0, 2.0], [2.0, 1.0], [3.0, 4.0], [4.0, 3.0]])
y = np.array([0, 1, 0, 1])
train_data = lgb.Dataset(X, label=y)
params = {'objective': 'binary', 'num_leaves': 3, 'min_data_in_leaf': 1, 'verbosity': -1}
booster = lgb.train(params, train_data, num_boost_round=1)
booster.save_model('normal_model.txt')

To test the heap over-read on master, just change num_leaves=3 to num_leaves=20 in the resulting text file and try to load it.

Also, this exact patch was already validated and merged into the ExaBoost fork by @BelixRogner (commit 9c7c4cb), confirming it hardens the parsing.

Let me know if you need anything else to move this forward.

[jameslamb]

Being merved into that unaffiliated fork does not provide any evidence that this patch is appropriate for LightGBM.

When someone reviews, we will alaso be looking at the impact on portability and performance.

[sanjay20m]

Understood, that makes total sense. I will be waiting for the maintainers' review.

Let me know if you need anything else from me when the time comes!