search

daphne-eu / daphne

DAPHNE: An Open and Extensible System Infrastructure for Integrated Data Analysis Pipelines
Apache License 2.0
67 stars 59 forks source link

Bounds checks for left and right indexing #572

Closed pdamme closed 8 months ago

pdamme commented 1 year ago

Left indexing (e.g., X[100:200, ] = ...) and right indexing (e.g., ... = X[100:200, ]) in DaphneDSL, respectively the ops/kernels backing them (InsertOp, ExtractOp, SliceOp, InsertRowOp, ExtractRowOp, SliceRowOp, InsertColOp, ExtractColOp, SliceColOp) do not check if the accessed positions are in bounds. This can lead to undefined behavior and crashes. This problem should be fixed by adding bounds checks in the kernels, and maybe already in the compiler (when possible).

AlexRTer commented 10 months ago

Hi, I'd like to be assigned to this issue please

pdamme commented 10 months ago

Sure, go ahead!

AlexRTer commented 9 months ago

I've experimented with adding a template parameter VTSel so that these OPs can receive negative indexes as well in order to return more accurate error messages. However, I believe daphne forwards these to the kernel as size_t when used in a .daph script anyway? So I'm wondering whether it is better to instead remove this template parameter again and simply check whether the value is beyond a certain threshold (ie. std::numeric_limits<size_t>::max() - 1000000) and add a "possible overflow" to the error message.

Furthermore, considering a mention in the existing code of possibly making these boundary validations and other checks optional: Would wrapping these in a #if(n)(def) ... #endif and setting a compiler flag such as -DNO_ERR_CHECK be a viable solution? If that seems reasonable I would open a new issue to discuss this.

pdamme commented 9 months ago

Regarding types: Indeed, the kernels for Slice(Row/Col)Op and Insert(Row/Col)Op hardcode the type of the position parameters to size_t. Introducing a template parameter for the positions (just like Extract(Row/Col)Op already do) makes sense. An instantiation with int64_t would allow us to pass negative numbers to the kernels.

Quickly looking into the code, I cannot find why DAPHNE should forward the numbers given in a DaphneDSL script to the kernels as size_ts. The left/right indexing expressions are parsed in src/parser/daphnedsl/DaphneDSLVisitor.cpp in DaphneDSLVisitor::visitRange() (see also the ANTLR4 grammar file src/parser/daphnedsl/DaphneDSLGrammar.g4 for an overview), and there are no hardcoded casts to a size type. If negative numbers don't reach the kernels, please double-check if you updated the kernel information in src/runtime/local/kernels/kernels.json by replacing the hardcoded size_t parameters of the respective kernels.

In general, I think it is a good idea to make the kernels accept signed position values, since

  1. it allows us to generate clearer error messages containing the values that were written in the DaphneDSL script in case of out-of-bounds accesses (as you mention above)
  2. we would like to support Python-style indexing with negative numbers in the future (but this is a separate issue, which has its own trickiness, see #235)

Regarding optional bounds checks: Yes, there is a comment on that in one of those kernels. Pre-compiler #ifs would be a viable approach to accomplish that. However, I think making the bounds checks optional would unnecessarily complicate the code for now and has unclear usefulness, so I recommend not making bounds checks optional :) .