diff --git a/reports/v415/clang-5.0/ReportStructure.md b/reports/v415/clang-5.0/ReportStructure.md new file mode 100644 index 0000000..b78032f --- /dev/null +++ b/reports/v415/clang-5.0/ReportStructure.md @@ -0,0 +1,18 @@ +# Structure of Reports and Explanation of Report Sections # +### Report Name & Number ### +Every report has an unique number. Every report name should follow this convention: +{subsystem_name}:{file_name}:{file_extension}:{warning_type}:{unique_report_number}.md +### General ### +General part contains: +**Warning Type:** Clang warning type. +**Warning Explanation:** Full clang warning message. +**File Location:** Full path of file, that causes warning. +### History ### +History part is about when and which commit introduced this warning and if it is already resolved or still exists. +**Introduced By:** Commit id and one-line commit message, that caused this warning. +**Reported Since:** Commit which makes this warning is observable through clang output. +**Resolved By:** Commit which resolves warning. +### Manuel Assesment ### +**Classification:** Warnings are classifed into different subclasses. Current subclasses can be found at [WarningTypeClassifications](WarningTypeClassifications.md) +**Rationale:** A summary of manual examination of warning. + diff --git a/reports/v415/clang-5.0/WarningTypeClassifications.md b/reports/v415/clang-5.0/WarningTypeClassifications.md new file mode 100644 index 0000000..d7c973d --- /dev/null +++ b/reports/v415/clang-5.0/WarningTypeClassifications.md @@ -0,0 +1,17 @@ +# Warning Type Classifications # + +Clang compiler creates a lot of warning while compiling Linux-Kernel. Some of them are false-positive , or they don't cause any problem right now, but in future, after a commit, they may create runtime problems. In here we try to classify clang warnings. + +### Mathematically Impossible ### +This classifier contains cases , that are impossible in mathematic operations, they are definitly false-positive warnings created by Clang compiler + +Such as : integer x : x < 10 && x > 9 + +### Tool can detect during compile time ### +In this subclass of Warnings, we have some code, that is not problematic right now but in the future with some new commits they may become problematic. +Such as : setting an array size to bigger than ```INT_MAX```. + +If we use a more-smarter tool, during runtime we can get information about these suspicious code lines will create a warning or not more accurately. + + + diff --git a/reports/v415/clang-5.0/drm:i915:i915_debugfs:c:sign-comparison:0007.md b/reports/v415/clang-5.0/drm:i915:i915_debugfs:c:sign-comparison:0007.md new file mode 100644 index 0000000..1cd7dad --- /dev/null +++ b/reports/v415/clang-5.0/drm:i915:i915_debugfs:c:sign-comparison:0007.md @@ -0,0 +1,66 @@ +# Analysis Report 0007 # + +## General ## +**Warning Type:** Wsign-compare +**Warning Explanation:**```drivers/gpu/drm/i915/i915_debugfs.c:4740:16: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare] + for (i = 0; i < ARRAY_SIZE(i915_debugfs_files); i++) {``` +**File Location:** drivers/gpu/drm/i915/i915_debugfs.c +## History ## +**Introduced By:** 34b9674c786c ("drm/i915: convert debugfs creation/destruction to table") +**Reported Since:** TODO +**Resolved By:** -- + +## Manuel Assesment ## +**Classification:** [Tool can detect during runtime](WarningTypeClassifications.md) +### Rationale ### +If we look at ```i915_debugfs_files``` array more carefully, we see that it is a static array, and at most it's size is 20. +So in this case ```ARRAY_SIZE(i915_debugfs_files)``` macro can't return value which is bigger than ```INT_MAX``` +```C +static const struct i915_debugfs_files { + const char *name; + const struct file_operations *fops; +} i915_debugfs_files[] = { + {"i915_wedged", &i915_wedged_fops}, + {"i915_max_freq", &i915_max_freq_fops}, + {"i915_min_freq", &i915_min_freq_fops}, + {"i915_cache_sharing", &i915_cache_sharing_fops}, + {"i915_ring_missed_irq", &i915_ring_missed_irq_fops}, + {"i915_ring_test_irq", &i915_ring_test_irq_fops}, + {"i915_gem_drop_caches", &i915_drop_caches_fops}, +#if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR) + {"i915_error_state", &i915_error_state_fops}, + {"i915_gpu_info", &i915_gpu_info_fops}, +#endif + {"i915_next_seqno", &i915_next_seqno_fops}, + {"i915_display_crc_ctl", &i915_display_crc_ctl_fops}, + {"i915_pri_wm_latency", &i915_pri_wm_latency_fops}, + {"i915_spr_wm_latency", &i915_spr_wm_latency_fops}, + {"i915_cur_wm_latency", &i915_cur_wm_latency_fops}, + {"i915_fbc_false_color", &i915_fbc_false_color_fops}, + {"i915_dp_test_data", &i915_displayport_test_data_fops}, + {"i915_dp_test_type", &i915_displayport_test_type_fops}, + {"i915_dp_test_active", &i915_displayport_test_active_fops}, + {"i915_guc_log_control", &i915_guc_log_control_fops}, + {"i915_hpd_storm_ctl", &i915_hpd_storm_ctl_fops}, + {"i915_ipc_status", &i915_ipc_status_fops}, + {"i915_drrs_ctl", &i915_drrs_ctl_fops} +}; + +int i915_debugfs_register(struct drm_i915_private *dev_priv) +{ + //... + int ret, i; + //... + for (i = 0; i < ARRAY_SIZE(i915_debugfs_files); i++) { + ent = debugfs_create_file(i915_debugfs_files[i].name, + S_IRUGO | S_IWUSR, + minor->debugfs_root, + to_i915(minor->dev), + i915_debugfs_files[i].fops); + if (!ent) + return -ENOMEM; + } + //... +} +``` +Clang couldn't evaulates this array size is not bigger than ```INT_MAX``` and creates a warning about it. Even if developer adds many elements to this list with another commit and list size become larger than ```INT_MAX```, a smart-tool can detect it during compile time. diff --git a/reports/v415/clang-5.0/drm:i915:i915_drv:c:sign-comparison:0006.md b/reports/v415/clang-5.0/drm:i915:i915_drv:c:sign-comparison:0006.md new file mode 100644 index 0000000..e8c82f1 --- /dev/null +++ b/reports/v415/clang-5.0/drm:i915:i915_drv:c:sign-comparison:0006.md @@ -0,0 +1,45 @@ +# Analysis Report 0006 # +## General ## +**Warning Type:** Wsign-compare +**Warning Explanation:** ```drivers/gpu/drm/i915/i915_drv.c:2234:16: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare] + for (i = 0; i < ARRAY_SIZE(s->lra_limits); i++) ``` +**File Location:** drivers/gpu/drm/i915/intel_pm.c + +## History ## +**Introduced By:** ddeea5b0c36f ("drm/i915: vlv: add runtime PM support") +**Reported Since:** TODO +**Resolved By:** -- +## Manuel Assesment ## +**Classification:** [Tool can detect during compile time](WarningTypeClassifications.md) +### Rationale ### +Clang compiler creates a warning about: +```C +static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv) +{ + struct vlv_s0ix_state *s = &dev_priv->vlv_s0ix_state; + int i; +//... + for (i = 0; i < ARRAY_SIZE(s->lra_limits); i++) +//... +}; +``` +In the for loop, there is a comparision between Array size of ```s->lra_limits``` and ```int i``` and Clang compiler finds it suspicious. + +To understand this ,we must look at ```struct vlv_s0ix_state``` which defined inside **drivers/gpu/drm/i915/i915_drv.h**: +```C +struct vlv_s0ix_state { + /* GAM */ + u32 wr_watermark; + u32 gfx_prio_ctrl; + u32 arb_mode; + u32 gfx_pend_tlb0; + u32 gfx_pend_tlb1; + u32 lra_limits[GEN7_LRA_LIMITS_REG_NUM]; +}; +``` +Developer defined lra_limits size as ```GEN_7_LRA_LIMITS_REG_NUM``` which is a static constant defined in **drivers/gpu/drm/i915/i915_reg.h** as following: +```C +#define GEN7_LRA_LIMITS_REG_NUM 13 +``` +As long as ```GEN7_LRA_LIMITS_REG_NUM``` is inside ```int``` limits, there won't be any problem theoratically. Even if developer increase ```GEN7_LRA_LIMITS_REG_NUM``` value to a problematic value, smart-tool can detect it during runtime. + diff --git a/reports/v415/clang-5.0/drm:i915:i915_gem_timeline:c:sign-comparison:0009.md b/reports/v415/clang-5.0/drm:i915:i915_gem_timeline:c:sign-comparison:0009.md new file mode 100644 index 0000000..bd2bd6d --- /dev/null +++ b/reports/v415/clang-5.0/drm:i915:i915_gem_timeline:c:sign-comparison:0009.md @@ -0,0 +1,43 @@ +# Analysis Report 0009 # +## General ## +**Warning Type:** Wsign-compare +**Warning Explanation:** ```drivers/gpu/drm/i915/i915_gem_timeline.c:124:17: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare] for (i = 0; i < ARRAY_SIZE(timeline->engine); i++) {``` +**File Location:** gpu/drivers/drm/i915/i915_gem_timeline.c +## History ## +**Introduced By:** d51dafaf07bf ("drm/i915: Assert all timeline requests are gone before fini") +**Reported Since:** TODO +**Resolved By:** -- +## Manuel Assesment ## +**Classification:** [Tool can detect during compile time](WarningTypeClassifications.md) +### Rationale ### +Clang creates a warning about: +```C +void i915_gem_timeline_fini(struct i915_gem_timeline *timeline) +{ + int i; + + lockdep_assert_held(&timeline->i915->drm.struct_mutex); + + for (i = 0; i < ARRAY_SIZE(timeline->engine); i++) + __intel_timeline_fini(&timeline->engine[i]); + + list_del(&timeline->link); + kfree(timeline->name); +} +``` +```struct i915_gem_timeline``` is defined in **gpu/drivers/gpu/drm/i915/i915_gem_timeline.h** as; +```C +struct i915_gem_timeline { + struct list_head link; + + struct drm_i915_private *i915; + const char *name; + + struct intel_timeline engine[I915_NUM_ENGINES]; +}; +``` +and value of ```I915_NUM_ENGINES``` defined in **gpu/drivers/gpu/drm/i915/i915_gem.h**; +```C +#define I915_NUM_ENGINES 5 +``` +As long as I915_NUM_ENGINES value is lower than ```INT_MAX``` this function is safe. Even if it become a larger value, a smart-tool can detect during compile time. diff --git a/reports/v415/clang-5.0/drm:i915:i915_gpu_error:c:sign-comparison:0004.md b/reports/v415/clang-5.0/drm:i915:i915_gpu_error:c:sign-comparison:0004.md new file mode 100644 index 0000000..56f316d --- /dev/null +++ b/reports/v415/clang-5.0/drm:i915:i915_gpu_error:c:sign-comparison:0004.md @@ -0,0 +1,49 @@ +# Analysis Report 0004 # +## General ## +**Warning Type:** Wsign-compare +**Warning Explanation:** ``drivers/gpu/drm/i915/i915_gpu_error.c:488:16: warning: comparison of integers of different signs: 'int' and 'const unsigned int' [-Wsign-compare] +for (n = 0; n < ee->num_ports; n++) {``` +**File Location:** drivers/gpu/drm/i915/i915_gpu_error.c + +## History ## +**Introduced By:** 76e70087d360 ("drm/i915: Make execlist port count variable") +**Reported Since:** TODO +**Resolved By:** -- + +## Manuel Assesment ## +**Classification:** [Tool can detect during compile time.](WarningTypeClassifications.md) +### Rationale ### +```C +//... +int n; +//... + for (n = 0; n < ee->num_ports; n++) { + err_printf(m, " ELSP[%d]:", n); + error_print_request(m, " ", &ee->execlist[n]); + } +``` + +Clang Compiler states that there is a problem inside for loop, about comparing ```int``` and ```unsigned int```. +```struct drm_i915_error_engine``` is defined in **drivers/gpu/drm/i915/i915_drv.h:502:** file as following: +```C +struct drm_i915_error_engine { +//... + struct drm_i915_error_request { + long jiffies; + pid_t pid; + u32 context; + int priority; + int ban_score; + u32 seqno; + u32 head; + u32 tail; + } *requests, execlist[EXECLIST_MAX_PORTS]; + unsigned int num_ports; +//.. +``` +Lastly ```EXECLIST_MAX_PORTS``` defined inside ***drivers/gpu/drm/i915/intel_ringbuffer.h***: +```C +#define EXECLIST_MAX_PORTS 2 +``` +As a summary, programmers defined EXECLIST_MAX_PORTS value by 2. Currently it is safe, and even if this value will be increased by a huge amount by a programmer in future, a smart-tool can detect it during compilation. + diff --git a/reports/v415/clang-5.0/drm:i915:intel_ddi:c:enum-conversion:0001.md b/reports/v415/clang-5.0/drm:i915:intel_ddi:c:enum-conversion:0001.md new file mode 100644 index 0000000..6caa3a5 --- /dev/null +++ b/reports/v415/clang-5.0/drm:i915:intel_ddi:c:enum-conversion:0001.md @@ -0,0 +1,110 @@ +# Analyis Report 0001 # + +## General ## +**Warning Type:** Wenum-conversion +**Warning Explanation:** Implicit conversion from enumeration type 'enum port' to different enumeration type 'enum intel_dpll_id' [-Wenum-conversion] +enum intel_dpll_id pll_id = port; +**File Location:** drivers/gpu/drm/i915/intel_ddi.c +## History ## +**Introduced By:** 2952cd6fb4cc ("drm/i915: Let's use more enum intel_dpll_id pll_id.") +**Reported Since:** 2952cd6fb4cc ("drm/i915: Let's use more enum intel_dpll_id pll_id.") +**Resolved By:** bb911536f07e ("drm/i915: Eliminate pll->state usage from bxt_calc_pll_link()"). +## Manuel Assesment ## +**Classification:** [Tool can detect during compile time](WarningTypeClassifications.md) +### Rational ### +```C +static void bxt_ddi_clock_get(struct intel_encoder *encoder, + struct intel_crtc_state *pipe_config) +{ + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); + enum port port = intel_ddi_get_encoder_port(encoder); + enum intel_dpll_id pll_id = port; //This line creates warning + pipe_config->port_clock = bxt_calc_pll_link(dev_priv, pll_id); + ddi_dotclock_get(pipe_config); +} +``` +There is an implicit conversion problem between enum intel_dpll_id and enum port. To clearly understand this problem, we must look definitions of enum port and enum intel_dpll_id: + +**enum intel_dpll_id:** +File location: /drm/i915/intel_dpll_mgr.h:48 +```C +enum intel_dpll_id { + /** + * @DPLL_ID_PRIVATE: non-shared dpll in use + */ + DPLL_ID_PRIVATE = -1, + /** + * @DPLL_ID_PCH_PLL_A: DPLL A in ILK, SNB and IVB + */ + DPLL_ID_PCH_PLL_A = 0, + /** + * @DPLL_ID_PCH_PLL_B: DPLL B in ILK, SNB and IVB + */ + DPLL_ID_PCH_PLL_B = 1, + /** + * @DPLL_ID_WRPLL1: HSW and BDW WRPLL1 + */ + DPLL_ID_WRPLL1 = 0, + /** + * @DPLL_ID_WRPLL2: HSW and BDW WRPLL2 + */ + DPLL_ID_WRPLL2 = 1, + /** + * @DPLL_ID_SPLL: HSW and BDW SPLL + */ + DPLL_ID_SPLL = 2, + /** + * @DPLL_ID_LCPLL_810: HSW and BDW 0.81 GHz LCPLL + */ + DPLL_ID_LCPLL_810 = 3, + /** + * @DPLL_ID_LCPLL_1350: HSW and BDW 1.35 GHz LCPLL + */ + DPLL_ID_LCPLL_1350 = 4, + /** + * @DPLL_ID_LCPLL_2700: HSW and BDW 2.7 GHz LCPLL + */ + DPLL_ID_LCPLL_2700 = 5, + /** + * @DPLL_ID_SKL_DPLL0: SKL and later DPLL0 + */ + DPLL_ID_SKL_DPLL0 = 0, + /** + * @DPLL_ID_SKL_DPLL1: SKL and later DPLL1 + */ + DPLL_ID_SKL_DPLL1 = 1, + /** + * @DPLL_ID_SKL_DPLL2: SKL and later DPLL2 + */ + DPLL_ID_SKL_DPLL2 = 2, + /** + * @DPLL_ID_SKL_DPLL3: SKL and later DPLL3 + */ + DPLL_ID_SKL_DPLL3 = 3, +}; +``` +**enum port:** +File location: /drm/i915/i915v\_drv.h:342 +```C +enum port { + PORT_NONE = -1, + PORT_A = 0, + PORT_B, + PORT_C, + PORT_D, + PORT_E, + I915_MAX_PORTS +}; +``` + +**Investigation of solution commit:** +Commit bb911536f07e modified the part of bxt_ddi_clock_get function , which was creating warning: +```diff +- struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); +- enum port port = encoder->port; +- enum intel_dpll_id pll_id = port; +- pipe_config->port_clock = bxt_calc_pll_link(dev_priv, pll_id); ++ pipe_config->port_clock = bxt_calc_pll_link(pipe_config); +``` + +So, as a result o this commit, there isn't any conversion between port and intel_dpll_id , and the enum-conversion warning is disappeared. diff --git a/reports/v415/clang-5.0/drm:i915:intel_display:c:sign-comparison:0008.md b/reports/v415/clang-5.0/drm:i915:intel_display:c:sign-comparison:0008.md new file mode 100644 index 0000000..5d6e391 --- /dev/null +++ b/reports/v415/clang-5.0/drm:i915:intel_display:c:sign-comparison:0008.md @@ -0,0 +1,47 @@ +# Analysis Report 0008 # +## General ## +**Warning Type:** Wsign-Compare +**Warning Explanation:** ```drivers/gpu/drm/i915/intel_display.c:13463:14: warning: comparison of integers of different signs: 'enum pipe' and 'unsigned long' [-Wsign-compare] + BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||``` +**File Location:** rivers/gpu/drm/i915/intel_display.c +## History ## +**Introduced By:** 22fd0fab3b51 ("drm/i915: pageflip fixes") +**Reported Since:** TODO +**Resolved By:** -- +## Manuel Assesment ## +**Classification:** [Tool can detect during compile time](WarningTypeClassifications.md) +### Rationale ### +Clang compiler creates a warning for part of ```intel_crtc_init``` function; +```C +static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe) +{ +//... + BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) || + dev_priv->plane_to_crtc_mapping[primary->i9xx_plane] != NULL); +//... +} +``` + +```struct drm_i915_private``` is defined in **drivers/gpu/drm/i915/i915_drv.h** file as following: +```C +struct drm_i915_private { +//... + struct intel_crtc *plane_to_crtc_mapping[I915_MAX_PIPES]; + struct intel_crtc *pipe_to_crtc_mapping[I915_MAX_PIPES]; +//... +} +``` +Also ```enum pipe``` and ```I915_MAX_PIPES``` are defined in **drivers/gpu/drm/i915/intel_display.h**; +```C +enum pipe { + INVALID_PIPE = -1, + + PIPE_A = 0, + PIPE_B, + PIPE_C, + _PIPE_EDP, + + I915_MAX_PIPES = _PIPE_EDP +}; +``` +```I915_MAX_PIPES``` is a constant value , and always equal to ```_PIPE_EDP``` value. In C , enum values are unsigned, so when pipe value equals to ```INVALID_PIPE``` it also evaluates ```true ``` in this comparison. As long as I915_MAX_PIPES equal to \_PIPE_EDP there won't be any problem. If it changes, a smart-tool can detect there may be a problem, during compile time. diff --git a/reports/v415/clang-5.0/drm:i915:intel_dpll_mgr:c:sign-comparison:0003.md b/reports/v415/clang-5.0/drm:i915:intel_dpll_mgr:c:sign-comparison:0003.md new file mode 100644 index 0000000..323350b --- /dev/null +++ b/reports/v415/clang-5.0/drm:i915:intel_dpll_mgr:c:sign-comparison:0003.md @@ -0,0 +1,58 @@ +# Analysis Report 0003 # +## General ## +**Warning Type:** Wsign-compare +**Warning Explanation:** ```drivers/gpu/drm/i915/intel_dpll_mgr.c:1239:18: warning: comparison of integers of different signs: 'unsigned int' and 'const int' [-Wsign-compare] +for (i = 0; i < dividers[d].n_dividers; i++) {``` +**File Location:** drivers/gpu/drm/i915/intel_dpll_mgr.c +## History ## +**Introduced By:** dc2538139277 ("drm/i915/skl: Replace the HDMI DPLL divider computation algorithm") +**Reported Since:** TODO +**Resolved By:** -- +## Manuel Assesment ## +**Classification:** [Mathematically Impossible](WarningTypeClassifications.md) +### Rationale ### +Clang creates a warning about +```C + static const int even_dividers[] = { 4, 6, 8, 10, 12, 14, 16, 18, 20, + 24, 28, 30, 32, 36, 40, 42, 44, + 48, 52, 54, 56, 60, 64, 66, 68, + 70, 72, 76, 78, 80, 84, 88, 90, + 92, 96, 98 }; + static const int odd_dividers[] = { 3, 5, 7, 9, 15, 21, 35 }; + static const struct { + const int *list; + int n_dividers; + } dividers[] = { + { even_dividers, ARRAY_SIZE(even_dividers) }, + { odd_dividers, ARRAY_SIZE(odd_dividers) }, + }; + struct skl_wrpll_context ctx; + unsigned int dco, d, i; + unsigned int p0, p1, p2; + + skl_wrpll_context_init(&ctx); + + for (d = 0; d < ARRAY_SIZE(dividers); d++) { + for (dco = 0; dco < ARRAY_SIZE(dco_central_freq); dco++) { + for (i = 0; i < dividers[d].n_dividers; i++) { + unsigned int p = dividers[d].list[i]; + uint64_t dco_freq = p * afe_clock; + + skl_wrpll_try_divider(&ctx, + dco_central_freq[dco], + dco_freq, + p); + /* + * Skip the remaining dividers if we're sure to + * have found the definitive divider, we can't + * improve a 0 deviation. + */ + if (ctx.min_deviation == 0) + goto skip_remaining_dividers; + } + } + } +``` +In this case , clang points out that ``` d ``` is an ``` unsigned int ```, however ```C dividers[d].n_dividers``` is an ```C int```. +Even though this case seems suspicious and actually problematic in theory. In practice, it won't cause any problems. Because the only risk is in here if ``` static const int even_dividers[] ``` or ``` static const int odd_dividers``` size is bigger than ```INT_SIZE``` ( > 2^31), then ``` int n_dividers``` will start get a negative value[integer overflow]. However in practice we cannot store a number which has more than 2^31 even or odd dividers in an ``` unsigned int```. +It is a **mathematically impossible** warning. diff --git a/reports/v415/clang-5.0/drm:i915:intel_guc_ct:c:sign-comparison:0010.md b/reports/v415/clang-5.0/drm:i915:intel_guc_ct:c:sign-comparison:0010.md new file mode 100644 index 0000000..602ed40 --- /dev/null +++ b/reports/v415/clang-5.0/drm:i915:intel_guc_ct:c:sign-comparison:0010.md @@ -0,0 +1,50 @@ +# Analysis Report 0010 # +#### Clang Compiler Warning #### +Sign Comparision +#### Warning Explanation #### +drivers/gpu/drm/i915/intel_guc_ct.c:162:16: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare] + for (i = 0; i < ARRAY_SIZE(ctch->ctbs); i++) { +#### Introduced By: f8a58d639dd9 ("drm/i915/guc: Introduce buffer based cmd transport") #### +#### Reported Since : 60d7a21aedad ("Merge tag 'nios2-v4.16-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/lftan/nios2") #### +#### File Location: drivers/gpu/drm/i915/intel_guc_ct.c #### +#### Solution Commit: -- #### + +#### Manuel Assesment: #### +Clang creates a warning about: +```C +static int ctch_init(struct intel_guc *guc, + struct intel_guc_ct_channel *ctch) +{ + //... + int i; + + /* store pointers to desc and cmds */ + for (i = 0; i < ARRAY_SIZE(ctch->ctbs); i++) { + GEM_BUG_ON((i != CTB_SEND) && (i != CTB_RECV)); + ctch->ctbs[i].desc = blob + PAGE_SIZE/4 * i; + ctch->ctbs[i].cmds = blob + PAGE_SIZE/4 * i + PAGE_SIZE/2; + } + //... +} +``` +```struct intel_guc_ct_channel``` defined in drm/i915/intel_guc_ct.h as; +```C +/** Represents pair of command transport buffers. + * + * Buffers go in pairs to allow bi-directional communication. + * To simplify the code we place both of them in the same vma. + * Buffers from the same pair must share unique owner id. + * + * @vma: pointer to the vma with pair of CT buffers + * @ctbs: buffers for sending(0) and receiving(1) commands + * @owner: unique identifier + * @next_fence: fence to be used with next send command + */ +struct intel_guc_ct_channel { + struct i915_vma *vma; + struct intel_guc_ct_buffer ctbs[2]; + u32 owner; + u32 next_fence; +}; +``` +In there we observe that programmers defined ctbs array with a static length, but clang couldn't find-out that value is static and raised a warning about this line. This line won't cause any problem during runtime. diff --git a/reports/v415/clang-5.0/drm:i915:intel_pm:c:sign-comparison:0005.md b/reports/v415/clang-5.0/drm:i915:intel_pm:c:sign-comparison:0005.md new file mode 100644 index 0000000..6c71491 --- /dev/null +++ b/reports/v415/clang-5.0/drm:i915:intel_pm:c:sign-comparison:0005.md @@ -0,0 +1,81 @@ +# Analysis Report 0005 # + +## General ## +**Warning Type:** Wsign-compare +**Warning Explanation:** ```drivers/gpu/drm/i915/intel_pm.c:9266:8: warning: comparison of integers of different signs: 'unsigned long long' and 'int' [-Wsign-compare] + ret = wait_for_atomic(COND, 50); ``` +**File Location:** drivers/gpu/drm/i915/intel_pm.c + +## History ## +**Introduced By:** a0b8a1fe3443 ("drm/i915/gen9: Fix PCODE polling during CDCLK change notification") +**Reported Since:** TODO +**Resolved By:** -- + +## Manuel Assesment ## +**Classification:** [Tool can detect during compile time](WarningTypeClassifications.md) +### Rationale ### +Clang compiler creates a warning about code part: +```C +int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request, + u32 reply_mask, u32 reply, int timeout_base_ms) +{ +//... +preempt_disable(); +ret = wait_for_atomic(COND, 50); +preempt_enable(); +//... +} +``` +Clang states there may be a sign comparision problem in a macro ```_wait_for_atomic```, which called from ```wait_for_atomic``` macro. +```wait_for_atomic``` macro defined in **drivers/gpu/drm/i915/intel_drv.h** as following: +```C +#define wait_for_atomic(COND, MS) wait_for_atomic_us((COND), (MS) * 1000) +``` +Basically ``` wait_for_atomic``` just calls another macro ```wait_for_atomic_us``` and in our case parameters as COND and 50000. +So lets look at ```wait_for_atomic_us``` macro which is defined in **drivers/gpu/drm/i915/intel_drv.h** as: +```C +#define wait_for_atomic_us(COND, US) \ +({ \ + BUILD_BUG_ON(!__builtin_constant_p(US)); \ + BUILD_BUG_ON((US) > 50000); \ + _wait_for_atomic((COND), (US), 1); \ +}) +``` +Just like before, it calls another macro ```_wait_for_atomic(COND, 50000, 1)```, and as final step we must look at ```_wait_for_atomic``` macro defination which is defined in **drivers/gpu/drm/i915/intel_drv.h** like others: +```C +#define _wait_for_atomic(COND, US, ATOMIC) \ +({ \ + int cpu, ret, timeout = (US) * 1000; \ + u64 base; \ + _WAIT_FOR_ATOMIC_CHECK(ATOMIC); \ + if (!(ATOMIC)) { \ + preempt_disable(); \ + cpu = smp_processor_id(); \ + } \ + base = local_clock(); \ + for (;;) { \ + u64 now = local_clock(); \ + if (!(ATOMIC)) \ + preempt_enable(); \ + if (COND) { \ + ret = 0; \ + break; \ + } \ + if (now - base >= timeout) { \ + ret = -ETIMEDOUT; \ + break; \ + } \ + cpu_relax(); \ + if (!(ATOMIC)) { \ + preempt_disable(); \ + if (unlikely(cpu != smp_processor_id())) { \ + timeout -= now - base; \ + cpu = smp_processor_id(); \ + base = local_clock(); \ + } \ + } \ + } \ + ret; \ +}) +``` +In this case, ```now - base``` substraction won't cause any problem during runtime. diff --git a/reports/v415/clang-5.0/drm:i915:intel_pm:c:sometimes-uninitialized:0002.md b/reports/v415/clang-5.0/drm:i915:intel_pm:c:sometimes-uninitialized:0002.md new file mode 100644 index 0000000..d958c60 --- /dev/null +++ b/reports/v415/clang-5.0/drm:i915:intel_pm:c:sometimes-uninitialized:0002.md @@ -0,0 +1,56 @@ +# Analysis Report 0002 # +## General ## +**Warning Type:** Wsometimes-uninitialized +**Warning Explanation:**```drivers/gpu/drm/i915/intel_pm.c:4670:6: warning: variable 'trans_min' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]``` +**File Location:** drivers/gpu/drm/i915/intel_pm.c +## History ## +**Introduced By:** ca47667f523e ("drm/i915/gen10: Calculate and enable transition WM") +**Reported Since:** TODO +**Resolved By:** -- +## Manuel Assesment ## +**Classification:** [Mathematically Impossible](WarningTypeClassifications.md) +### Rational ### +Clang creates a warning about code part: +```C + /* Transition WM are not recommended by HW team for GEN9 */ + if (INTEL_GEN(dev_priv) <= 9) + goto exit; + + /* Transition WM don't make any sense if ipc is disabled */ + if (!dev_priv->ipc_enabled) + goto exit; + + if (INTEL_GEN(dev_priv) >= 10) + trans_min = 4; + + trans_offset_b = trans_min + trans_amount; + + //... + +exit: + trans_wm->plane_en = false; +``` + +``` INTEL_GEN ``` is a macro defined in **drivers/gpu/drm/i915/i915_drv.h:2939** as following: +```C +#define INTEL_GEN(dev_priv) ((dev_priv)->info.gen) +``` +So basically it is just a getter function that returns ``` drm_i915_private->info->gen``` +``` struct drm_i915_private ``` defined in **drivers/gpu/drm/i915/i915_drv.h:2233** as: +```C +struct drm_i915_private { +//... +const struct intel_device_info info; +//... +} +``` +As a third step, we must check ``` intel_device_info.gen ```'s type +``` struct intel_device_info ``` defined in **drivers/gpu/drm/i915/i915_drv.h:853** as: +```C +struct intel_device_info { +//... +u8 gen; +//... +}; +``` +As a result of this observations since ```intel_device_info.gen``` is a u8 type, it always be initialized in this code structure. It won't create any problems theoretically. It is [**mathmetically impossible**](WarningTypeClassifications.md). diff --git a/reports/v415/infer-0131/InferReport0001:lib:list_sort:c:memoryleak.md b/reports/v415/infer-0131/InferReport0001:lib:list_sort:c:memoryleak.md new file mode 100644 index 0000000..f0bda47 --- /dev/null +++ b/reports/v415/infer-0131/InferReport0001:lib:list_sort:c:memoryleak.md @@ -0,0 +1,87 @@ +# Analysis Report 0001 # + +## General ## +**Warning Type:** Memory Leak +**Warning Explanation:** +``` + + `tail->next` is not reachable after line 27, column 4. + 25. /* if equal, take 'a' -- important for sort stability */ + 26. if ((*cmp)(priv, a, b) <= 0) { + 27. > tail->next = a; + 28. a = a->next; + 29. } else { +``` +``` + + `tail->next` is not reachable after line 30, column 4. + 28. a = a->next; + 29. } else { + 30. > tail->next = b; + 31. b = b->next; + 32. } +``` +**File Location:** lib/list_sort.c +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- +## Manuel Assesment ## +**Classification:** Not sure author did this for optimization, but in my opinion there may be a memory leak. +### Rationale ### +In this example, after assignment infer thinks we cannot get tail->next's previous reference anyway. So it suggests first we should free tail->next value and than assign it to a, or b. + +I have created a minimal example for this case as following: +```C +#include +#include +struct list_element { + struct list_element *next, *prev; + int element; +}; + +int change(struct list_element *a, struct list_element *b) { + struct list_element head, *tail = &head; + while(a && b) { + if(a->element > b->element) { + //free(tail->next); + tail->next = a; + a = a->next; + } else { + //free(tail->next); + tail->next = b; + b = b->next; + } + tail = tail->next; + } + tail->next = a?:b; + return 1; +} + + +int main() { + struct list_element l1 = {NULL, NULL, 5}; + struct list_element l2 = {NULL, NULL ,4}; + change(&l1, &l2); + return 0; +} +``` +With this piece of code, Infer creates an error which is very similar to its output on lib/list_sort.c: +``` +list_sort.c:13: error: MEMORY_LEAK + `tail->next` is not reachable after line 13, column 4. + 11. if(a->element > b->element) { + 12. //free(tail->next); + 13. > tail->next = a; + 14. a = a->next; + 15. } else { + +list_sort.c:17: error: MEMORY_LEAK + `tail->next` is not reachable after line 17, column 4. + 15. } else { + 16. //free(tail->next); + 17. > tail->next = b; + 18. b = b->next; + 19. } +``` +However if I delete comments that are placed in front of ```free``` function, Infer won't show any warnings. Actually since this code modified optimized in commit: 835cc0c8477f(lib: more scalable list_sort()), I want to be sure about that author did this in aware of this problem, but wants to get a better speed. diff --git a/reports/v415/infer-0131/InferReport0002:tty:tty_jobctrl:c:memoryleak.md b/reports/v415/infer-0131/InferReport0002:tty:tty_jobctrl:c:memoryleak.md new file mode 100644 index 0000000..7b5fb46 --- /dev/null +++ b/reports/v415/infer-0131/InferReport0002:tty:tty_jobctrl:c:memoryleak.md @@ -0,0 +1,87 @@ +# Analysis Report 0002 # + +## General ## +**Warning Type:** Memory Leak +**Warning Explanation:** +``` + `tty->pgrp` is not reachable after line 105, column 2. + 103. put_pid(tty->session); + 104. put_pid(tty->pgrp); + 105. > tty->pgrp = get_pid(task_pgrp(current)); + 106. spin_unlock_irqrestore(&tty->ctrl_lock, flags); + 107. tty->session = get_pid(task_session(current)); +``` +**File Location:** drivers/tty/tty_jobctrl.c +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- +## Manuel Assesment ## +**Classification:** Looks like false-positive but not make sure right now. +### Rationale ### +Even though infer claims that, we won't use tty->pgrp after line 105, if we analyze this function more detailed: +```C +static void __proc_set_tty(struct tty_struct *tty) +{ + unsigned long flags; + + spin_lock_irqsave(&tty->ctrl_lock, flags); + /* + * The session and fg pgrp references will be non-NULL if + * tiocsctty() is stealing the controlling tty + */ + put_pid(tty->session); + put_pid(tty->pgrp); + tty->pgrp = get_pid(task_pgrp(current)); + spin_unlock_irqrestore(&tty->ctrl_lock, flags); + tty->session = get_pid(task_session(current)); + if (current->signal->tty) { + tty_debug(tty, "current tty %s not NULL!!\n", + current->signal->tty->name); + tty_kref_put(current->signal->tty); + } + put_pid(current->signal->tty_old_pgrp); + current->signal->tty = tty_kref_get(tty); + current->signal->tty_old_pgrp = NULL; +} +``` +Actually we refer this new tty->pgrp by line ```current->signal->tty = tty_kref_get(tty)``` and after this function completes, Linux will start use this tty->pgrp in ```current``` which is defined in ```include/linux/sched.h``` as: +```C +struct task_struct { +.... + /* Signal handlers: */ + struct signal_struct *signal; + struct sighand_struct *sighand; +..... + +}; +``` +and in addition to that in arch directory every subdirectory has its own current.h file, for example in x86 subdirectory: +``` +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ASM_X86_CURRENT_H +#define _ASM_X86_CURRENT_H + +#include +#include + +#ifndef __ASSEMBLY__ +struct task_struct; + +DECLARE_PER_CPU(struct task_struct *, current_task); + +static __always_inline struct task_struct *get_current(void) +{ + return this_cpu_read_stable(current_task); +} + +#define current get_current() + +#endif /* __ASSEMBLY__ */ + +#endif /* _ASM_X86_CURRENT_H */ +``` +So in my opinion change in a data , which is using by task_struct current, is cannot be classified as not-reachable. + + + diff --git a/reports/v415/infer-0131/InferReport0003:md:dm-kcopyd:c:memoryleak.md b/reports/v415/infer-0131/InferReport0003:md:dm-kcopyd:c:memoryleak.md new file mode 100644 index 0000000..85f563b --- /dev/null +++ b/reports/v415/infer-0131/InferReport0003:md:dm-kcopyd:c:memoryleak.md @@ -0,0 +1,107 @@ +# Analysis Report 0003 # + +## General ## +**Warning Type:** Memory Leak +**Warning Explanation:** +``` + `pl->next` is not reachable after line 256, column 3. + 254. kc->nr_free_pages--; + 255. } + 256. > pl->next = *pages; + 257. *pages = pl; + 258. } while (--nr); +``` +``` + `next->next` is not reachable after line 297, column 3. + 295. return -ENOMEM; + 296. } + 297. > next->next = pl; + 298. pl = next; + 299. } +``` +**File Location:** drivers/md/dm-kcopyd.c +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- +## Manuel Assesment ## +**Classification:** First one makes too much function calls, look more carefully, I think second one is false-positive. +### Rationale ### +#### First Part #### +```C +static int kcopyd_get_pages(struct dm_kcopyd_client *kc, + unsigned int nr, struct page_list **pages) +{ + struct page_list *pl; + + *pages = NULL; + + do { + pl = alloc_pl(__GFP_NOWARN | __GFP_NORETRY | __GFP_KSWAPD_RECLAIM); + if (unlikely(!pl)) { + /* Use reserved pages */ + pl = kc->pages; + if (unlikely(!pl)) + goto out_of_memory; + kc->pages = pl->next; + kc->nr_free_pages--; + } + pl->next = *pages; + *pages = pl; + } while (--nr); + ..... +} +``` +To understand both of these two problems better, first we must look at dm_kcopyd_client and page_list structs. +```C +struct dm_kcopyd_client { + struct page_list *pages; + unsigned nr_reserved_pages; + unsigned nr_free_pages; +...... +}; +``` +```C +struct page_list { + struct page_list *next; + struct page *page; +}; +``` +Now make a summary of investigation and write it here + +#### Second Part #### +```C +for (i = 0; i < nr_pages; i++) { + next = alloc_pl(GFP_KERNEL); + if (!next) { + if (pl) + drop_pages(pl); + return -ENOMEM; + } + next->next = pl; + pl = next; + } +``` +Again infer is suspicious about a memory leak in here. According to my observation, it analyzes as: +during ``` next = alloc_pl(GFP_KERNEL); ``` if there is a data linked to next->next, then we cannot access it anymore using structs. However if we look at ```alloc_pl``` function, +```C +static struct page_list *alloc_pl(gfp_t gfp) +{ + struct page_list *pl; + + pl = kmalloc(sizeof(*pl), gfp); + if (!pl) + return NULL; + + pl->page = alloc_page(gfp); + if (!pl->page) { + kfree(pl); + return NULL; + } + + return pl; +} +``` +It doesn't change anything on next->next pointer. It only changes next->page, but it is irrelevant to our warning case. +So in my opinion second warning is False-Positive + diff --git a/reports/v415/infer-0131/InferReport0004:lib:radix-tree:c:memoryleak.md b/reports/v415/infer-0131/InferReport0004:lib:radix-tree:c:memoryleak.md new file mode 100644 index 0000000..8c5c09d --- /dev/null +++ b/reports/v415/infer-0131/InferReport0004:lib:radix-tree:c:memoryleak.md @@ -0,0 +1,77 @@ +# Analysis Report 0004 # + +## General ## +**Warning Type:** Memory Leak +**Warning Explanation:** +``` + `node->parent` is not reachable after line 488, column 4. + 486. rtp = this_cpu_ptr(&radix_tree_preloads); + 487. if (rtp->nr < nr) { + 488. > node->parent = rtp->nodes; + 489. rtp->nodes = node; + 490. rtp->nr++; +``` +**File Location:** lib/radix-tree.c +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- +## Manuel Assesment ## +**Classification:** Highly Possible, but try to reproduce it with smaller code examples. +### Rationale ### +Here Infer warns about that, after this struct's pointer links changed, there will be a piece of data which is still in memory, but cannot be reachable through any of this structs. +So let's look at that. +```C +static __must_check int __radix_tree_preload(gfp_t gfp_mask, unsigned nr) +{ + struct radix_tree_preload *rtp; + struct radix_tree_node *node; +.... + while (rtp->nr < nr) { + preempt_enable(); + node = kmem_cache_alloc(radix_tree_node_cachep, gfp_mask); + if (node == NULL) + goto out; + preempt_disable(); + rtp = this_cpu_ptr(&radix_tree_preloads); + if (rtp->nr < nr) { + node->parent = rtp->nodes; + rtp->nodes = node; + rtp->nr++; + } else { + .... + ``` +rtp is a type of ```radix_tree_preload``` struct whereas node is a ```radix_tree_node``` struct. +```C +struct radix_tree_node { + unsigned char shift; /* Bits remaining in each slot */ + unsigned char offset; /* Slot offset in parent */ + unsigned char count; /* Total entry count */ + unsigned char exceptional; /* Exceptional entry count */ + struct radix_tree_node *parent; /* Used when ascending tree */ + struct radix_tree_root *root; /* The tree we belong to */ +}; +``` +```C +struct radix_tree_preload { + unsigned nr; + /* nodes->parent points to next preallocated node */ + struct radix_tree_node *nodes; +}; +``` +As we can observer, radix_tree_node struct doesn not only has links to its parent and root, but also encapsulates different information too. +According to infer tricky part of the code is: if ```node``` points to a memory block which has shift, offset and count data. However +``` node->parent = rtp-> nodes; + rtp->nodes = node; +``` +If before this two line, lets say node->parent = Y , and rtp -> nodes = X +after this two lines executed, node->parent = X and rtp -> nodes = node, +we couldn't reach to Y using this structs anymore. So Infer suggests we must free(Y) before starting this operations. + + + + + + + + diff --git a/reports/v415/infer-0131/InferReport0005:time:posix-timers:c:memoryleak.md b/reports/v415/infer-0131/InferReport0005:time:posix-timers:c:memoryleak.md new file mode 100644 index 0000000..51fbe08 --- /dev/null +++ b/reports/v415/infer-0131/InferReport0005:time:posix-timers:c:memoryleak.md @@ -0,0 +1,73 @@ +# Analysis Report 0005 # + +## General ## +**Warning Type:** Memory Leak +**Warning Explanation:** +``` +`return` is not reachable after line 586, column 3. + 584. if (copy_from_user(&event, timer_event_spec, sizeof (event))) + 585. return -EFAULT; + 586. > return do_timer_create(which_clock, &event, created_timer_id); + 587. } + 588. return do_timer_create(which_clock, NULL, created_timer_id); +``` +``` +`return` is not reachable after line 588, column 2. + 586. return do_timer_create(which_clock, &event, created_timer_id); + 587. } + 588. > return do_timer_create(which_clock, NULL, created_timer_id); + 589. } + 590. +``` +``` +`return` is not reachable after line 601, column 3. + 599. if (get_compat_sigevent(&event, timer_event_spec)) + 600. return -EFAULT; + 601. > return do_timer_create(which_clock, &event, created_timer_id); + 602. } + 603. return do_timer_create(which_clock, NULL, created_timer_id); +``` +``` +`return` is not reachable after line 603, column 2. + 601. return do_timer_create(which_clock, &event, created_timer_id); + 602. } + 603. > return do_timer_create(which_clock, NULL, created_timer_id); + 604. } + 605. #endif +``` + +**File Location:** kernel/time/posix-timers.c +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- +## Manuel Assesment ## +**Classification:** STILL NOT SURE, +### Rationale ### +I will investigate errors in two groups since 1 & 2 in the SYSCALL_DEFINE3 function and 3 & 4 in COMPAT_SYSCALL_DEFINE3 function. +#### Error 1 & 2 ##### +Lets start with SYSCALL_DEFINE3 function, Infer claims that this function always exists with return -EFAULT. +``` +SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock, + struct sigevent __user *, timer_event_spec, + timer_t __user *, created_timer_id) +{ + if (timer_event_spec) { + sigevent_t event; + + if (copy_from_user(&event, timer_event_spec, sizeof (event))) + return -EFAULT; + return do_timer_create(which_clock, &event, created_timer_id); + } + return do_timer_create(which_clock, NULL, created_timer_id); +} +``` +To understand current workflow in there, lets look at what is timer_event_spec ,which is inside include/linux/compat.h as: +``` +asmlinkage long compat_sys_timer_create(clockid_t which_clock, + struct compat_sigevent __user *timer_event_spec, + timer_t __user *created_timer_id); +``` + +Continue modifiying current notes more readable and suitable for report structure. + diff --git a/reports/v415/infer-0131/InferReport0006:ipc:msg:c:memoryleak.md b/reports/v415/infer-0131/InferReport0006:ipc:msg:c:memoryleak.md new file mode 100644 index 0000000..0ed8ce4 --- /dev/null +++ b/reports/v415/infer-0131/InferReport0006:ipc:msg:c:memoryleak.md @@ -0,0 +1,85 @@ +# Analysis Report 0006 # + +## General ## +**Warning Type:** Memory Leak +**Warning Explanation:** +``` + `msg` is not reachable after line 750, column 2. + 748. return -EINVAL; + 749. + 750. > msg = load_msg(mtext, msgsz); + 751. if (IS_ERR(msg)) + 752. return PTR_ERR(msg); +``` +**File Location:** ipc/msg.c +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- +## Manuel Assesment ## +**Classification:** FALSE-POSITIVE +### Rationale ## +We must look at the function from beginning of it, to understand that warning better. +```C +static long do_msgsnd(int msqid, long mtype, void __user *mtext, + size_t msgsz, int msgflg) +{ + struct msg_queue *msq; + struct msg_msg *msg; + int err; + struct ipc_namespace *ns; + DEFINE_WAKE_Q(wake_q); + + ns = current->nsproxy->ipc_ns; + + if (msgsz > ns->msg_ctlmax || (long) msgsz < 0 || msqid < 0) + return -EINVAL; + if (mtype < 1) + return -EINVAL; + + msg = load_msg(mtext, msgsz); + if (IS_ERR(msg)) + return PTR_ERR(msg); + ... +``` +In this case, Infer warns us about we allocate space to msg_msg *msg via ```load_msg``` function. However if there will be a memory leak, if we return directly without free the space that allocated to *msg. +Lets look at ```load_msg``` part +```C +struct msg_msg *load_msg(const void __user *src, size_t len) +{ + struct msg_msg *msg; + struct msg_msgseg *seg; + int err = -EFAULT; + size_t alen; + + msg = alloc_msg(len); + if (msg == NULL) + return ERR_PTR(-ENOMEM); + + alen = min(len, DATALEN_MSG); + if (copy_from_user(msg + 1, src, alen)) + goto out_err; + + for (seg = msg->next; seg != NULL; seg = seg->next) { + len -= alen; + src = (char __user *)src + alen; + alen = min(len, DATALEN_SEG); + if (copy_from_user(seg + 1, src, alen)) + goto out_err; + } + + err = security_msg_msg_alloc(msg); + if (err) + goto out_err; + + return msg; + +out_err: + free_msg(msg); + return ERR_PTR(err); +} +``` +As you can see here, if msg is not null, then programmer first free msg and then return. Only in ```msg == null``` programmer returned directly, and if msg equals to null that means we couldn't allocated space. So in my opinion there won't be a memory leak during execution of this function. + + +http://home.netcom.com/%7Etjensen/ptr/pointers.htm diff --git a/reports/v415/infer-0131/InferReport0007:irq:manage:c:memoryleak.md b/reports/v415/infer-0131/InferReport0007:irq:manage:c:memoryleak.md new file mode 100644 index 0000000..2315fd8 --- /dev/null +++ b/reports/v415/infer-0131/InferReport0007:irq:manage:c:memoryleak.md @@ -0,0 +1,69 @@ +# Analysis Report 0007 # + +## General ## +**Warning Type:** Memory Leak +**Warning Explanation:** +``` + 'chip' is not reachable after line 2154, column 3. + 2152. + 2153. if (data) + 2154. err = chip->irq_get_irqchip_state(data, which, state); + 2155. + 2156. irq_put_desc_busunlock(desc, flags); +``` +``` + `chip` is not reachable after line 2200, column 3. + 2198. + 2199. if (data) + 2200. > err = chip->irq_set_irqchip_state(data, which, val); + 2201. + 2202. irq_put_desc_busunlock(desc, flags); +``` +**File Location:** kernel/irq/manage.c +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- +## Manuel Assesment ## +**Classification:** False-Positive +### Rationale ### +#### First Error - Line 2154 #### +If we look at full content of related block of code +```C +int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which, + bool val) +{ + struct irq_desc *desc; + struct irq_data *data; + struct irq_chip *chip; + unsigned long flags; + int err = -EINVAL; + + desc = irq_get_desc_buslock(irq, &flags, 0); + if (!desc) + return err; + + data = irq_desc_get_irq_data(desc); + + do { + chip = irq_data_get_irq_chip(data); + if (chip->irq_set_irqchip_state) + break; +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY + data = data->parent_data; +#else + data = NULL; +#endif + } while (data); + + if (data) + err = chip->irq_set_irqchip_state(data, which, val); + + irq_put_desc_busunlock(desc, flags); + return err; +} +``` +If desc doesn't have allocated space, this function will return at ```if(!desc)``` check. +and since we have a do {} while loop, It is guaranteed that, this loop will be executed at least once. +I think it is a good idea to call ```free(chip);``` before ```return err;``` + diff --git a/reports/v415/infer-0131/InferReport0008:selinux:hooks:c:memoryleak.md b/reports/v415/infer-0131/InferReport0008:selinux:hooks:c:memoryleak.md new file mode 100644 index 0000000..705a683 --- /dev/null +++ b/reports/v415/infer-0131/InferReport0008:selinux:hooks:c:memoryleak.md @@ -0,0 +1,83 @@ +# Analysis Report 0008 # + +## General ## +**Warning Type:** Memory Leak +**Warning Explanation:** +``` + `return` is not reachable after line 3149, column 3. + 3147. + 3148. if (cap_capable(cred, &init_user_ns, CAP_MAC_ADMIN, cap_audit)) + 3149. > return false; + 3150. if (cred_has_capability(cred, CAP_MAC_ADMIN, cap_audit, true)) + 3151. return false; +``` +**File Location:** security/selinux/hooks.c +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- +## Manuel Assesment ## +**Classification:** False-Positive +### Rationale ### +```C +static bool has_cap_mac_admin(bool audit) +{ + const struct cred *cred = current_cred(); + int cap_audit = audit ? SECURITY_CAP_AUDIT : SECURITY_CAP_NOAUDIT; + + if (cap_capable(cred, &init_user_ns, CAP_MAC_ADMIN, cap_audit)) + return false; + if (cred_has_capability(cred, CAP_MAC_ADMIN, cap_audit, true)) + return false; + return true; +} +``` +To understand if it is false positive or not, we must check ```cap_capable``` functions return values. +It is located in: security/commoncap.c file, and defined as: +```C +int cap_capable(const struct cred *cred, struct user_namespace *targ_ns, + int cap, int audit) +{ + struct user_namespace *ns = targ_ns; + + /* See if cred has the capability in the target user namespace + * by examining the target user namespace and all of the target + * user namespace's parents. + */ + for (;;) { + /* Do we have the necessary capabilities? */ + if (ns == cred->user_ns) + return cap_raised(cred->cap_effective, cap) ? 0 : -EPERM; + + /* + * If we're already at a lower level than we're looking for, + * we're done searching. + */ + if (ns->level <= cred->user_ns->level) + return -EPERM; + + /* + * The owner of the user namespace in the parent of the + * user namespace has all caps. + */ + if ((ns->parent == cred->user_ns) && uid_eq(ns->owner, cred->euid)) + return 0; + + /* + * If you have a capability in a parent user ns, then you have + * it over all children user namespaces as well. + */ + ns = ns->parent; + } + + /* We never get here */ +} +``` +It may return either ```0``` or ```-EPERM``` value. We need to look at ```EPERM``` value more carefully. This value defined in: +include/uapi/asm-generic/errno-base.h file as: +``` #define EPERM 1 /* Operation not permitted */``` + +It is very obvious that this function will return ```-EPERM``` value if user doesn't has capability, which that lead an evaluation of +```if (cap_capable(cred, &init_user_ns, CAP_MAC_ADMIN, cap_audit))``` function true. However Infer couldn't detect that and raises an error about memory leak. + + diff --git a/reports/v415/infer-0131/InferReport0009:objtool:orc_dump:c:resourceleak.md b/reports/v415/infer-0131/InferReport0009:objtool:orc_dump:c:resourceleak.md new file mode 100644 index 0000000..73a35ab --- /dev/null +++ b/reports/v415/infer-0131/InferReport0009:objtool:orc_dump:c:resourceleak.md @@ -0,0 +1,179 @@ +# Analysis Report 0009 # + +## General ## +**Warning Type:** Resource Leak +**Warning Explanation:** +``` + resource acquired by call to `open()` at line 94, column 7 is not released after line 102, column 3. + 100. elf = elf_begin(fd, ELF_C_READ_MMAP, NULL); + 101. if (!elf) { + 102. > WARN_ELF("elf_begin"); + 103. return -1; + 104. } +``` +``` + resource acquired by call to `open()` at line 94, column 7 is not released after line 107, column 3. + 105. + 106. if (elf_getshdrnum(elf, &nr_sections)) { + 107. > WARN_ELF("elf_getshdrnum"); + 108. return -1; + 109. } +``` +``` + resource acquired by call to `open()` at line 94, column 7 is not released after line 112, column 3. + 110. + 111. if (elf_getshdrstrndx(elf, &shstrtab_idx)) { + 112. > WARN_ELF("elf_getshdrstrndx"); + 113. return -1; + 114. } +``` +``` + resource acquired by call to `open()` at line 94, column 7 is not released after line 119, column 4. + 117. scn = elf_getscn(elf, i); + 118. if (!scn) { + 119. > WARN_ELF("elf_getscn"); + 120. return -1; + 121. } +``` +``` + resource acquired by call to `open()` at line 94, column 7 is not released after line 124, column 4. + 122. + 123. if (!gelf_getshdr(scn, &sh)) { + 124. > WARN_ELF("gelf_getshdr"); + 125. return -1; + 126. } +``` +``` + resource acquired by call to `open()` at line 94, column 7 is not released after line 130, column 4. + 128. name = elf_strptr(elf, shstrtab_idx, sh.sh_name); + 129. if (!name) { + 130. > WARN_ELF("elf_strptr"); + 131. return -1; + 132. } +``` +``` + resource acquired by call to `open()` at line 94, column 7 is not released after line 136, column 4. + 134. data = elf_getdata(scn, NULL); + 135. if (!data) { + 136. > WARN_ELF("elf_getdata"); + 137. return -1; + 138. } + +``` +**File Location:** tools/objtool/orc_dump.c +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- +## Manuel Assesment ## +**Classification:** It is an obvious error, but maybe programmer do that on purpose, I should continue to read documentation. +### Rationale ### +Facebook infer creates resource leak errors about, a file opened , however function exists without closing it and it may lead an resource leak. +Lets look at this function more detailed: +```C +int orc_dump(const char *_objname) +{ +.... + fd = open(objname, O_RDONLY); + if (fd == -1) { + perror("open"); + return -1; + } + + elf = elf_begin(fd, ELF_C_READ_MMAP, NULL); + if (!elf) { + WARN_ELF("elf_begin"); + return -1; + } + + if (elf_getshdrnum(elf, &nr_sections)) { + WARN_ELF("elf_getshdrnum"); + return -1; + } + + if (elf_getshdrstrndx(elf, &shstrtab_idx)) { + WARN_ELF("elf_getshdrstrndx"); + return -1; + } + + for (i = 0; i < nr_sections; i++) { + scn = elf_getscn(elf, i); + if (!scn) { + WARN_ELF("elf_getscn"); + return -1; + } + + if (!gelf_getshdr(scn, &sh)) { + WARN_ELF("gelf_getshdr"); + return -1; + } + + name = elf_strptr(elf, shstrtab_idx, sh.sh_name); + if (!name) { + WARN_ELF("elf_strptr"); + return -1; + } + + data = elf_getdata(scn, NULL); + if (!data) { + WARN_ELF("elf_getdata"); + return -1; + } + ..... + ..... + if (!symtab || !orc || !orc_ip) + return 0; + + if (orc_size % sizeof(*orc) != 0) { + WARN("bad .orc_unwind section size"); + return -1; + } + + nr_entries = orc_size / sizeof(*orc); + for (i = 0; i < nr_entries; i++) { + if (rela_orc_ip) { + if (!gelf_getrela(rela_orc_ip, i, &rela)) { + WARN_ELF("gelf_getrela"); + return -1; + } + + if (!gelf_getsym(symtab, GELF_R_SYM(rela.r_info), &sym)) { + WARN_ELF("gelf_getsym"); + return -1; + } + + scn = elf_getscn(elf, sym.st_shndx); + if (!scn) { + WARN_ELF("elf_getscn"); + return -1; + } + + if (!gelf_getshdr(scn, &sh)) { + WARN_ELF("gelf_getshdr"); + return -1; + } + + name = elf_strptr(elf, shstrtab_idx, sh.sh_name); + if (!name || !*name) { + WARN_ELF("elf_strptr"); + return -1; + } + + printf("%s+%llx:", name, (unsigned long long)rela.r_addend); + + } else { + printf("%llx:", (unsigned long long)(orc_ip_addr + (i * sizeof(int)) + orc_ip[i])); + } +..... + } + + elf_end(elf); + close(fd); + + return 0; +} +``` +As it can easily observed, in the ```int orc dump()``` function programmer opens a file using ```open(objname, O_RDONLY);``` function +However after that he/she makes different control to be sure about that workflow is going well, but if he/she find a problem, exits with only +printing ```WARN_ELF``` and then returns -1. In my opinion before return -1, he/she should close file. However since it is a really obvious error, +I have some doubts about , programmer does that on purpose. I read some documentation about objtool , however couldn't find a section about this topic. diff --git a/reports/v416/infer-014/drivers:usb:core:hub:c:uninitializedvalue:0004.md b/reports/v416/infer-014/drivers:usb:core:hub:c:uninitializedvalue:0004.md new file mode 100644 index 0000000..94424c0 --- /dev/null +++ b/reports/v416/infer-014/drivers:usb:core:hub:c:uninitializedvalue:0004.md @@ -0,0 +1,113 @@ +# Analysis Report 0004 # + +## General ## +**Warning Type:** UNINITIALIZED_VALUE +**Warning Explanation:** The value read from portstatus was never initialized. +```C + if (stable_time < HUB_DEBOUNCE_STABLE) + return -ETIMEDOUT; + return portstatus; + } + +``` +**File Location:** drivers/usb/core/hub.c:4270 +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- + +## Manuel Assesment ## +**Classification:** FALSE-POSITIVE +### Rationale ### +Infer raises an uninitialized value error about function: +```C +int hub_port_debounce(struct usb_hub *hub, int port1, bool must_be_connected) +{ + int ret; + u16 portchange, portstatus; + unsigned connection = 0xffff; + int total_time, stable_time = 0; + struct usb_port *port_dev = hub->ports[port1 - 1]; + + for (total_time = 0; ; total_time += HUB_DEBOUNCE_STEP) { + ret = hub_port_status(hub, port1, &portstatus, &portchange); + if (ret < 0) + return ret; + + if (!(portchange & USB_PORT_STAT_C_CONNECTION) && + (portstatus & USB_PORT_STAT_CONNECTION) == connection) { + if (!must_be_connected || + (connection == USB_PORT_STAT_CONNECTION)) + stable_time += HUB_DEBOUNCE_STEP; + if (stable_time >= HUB_DEBOUNCE_STABLE) + break; + } else { + stable_time = 0; + connection = portstatus & USB_PORT_STAT_CONNECTION; + } + + if (portchange & USB_PORT_STAT_C_CONNECTION) { + usb_clear_port_feature(hub->hdev, port1, + USB_PORT_FEAT_C_CONNECTION); + } + + if (total_time >= HUB_DEBOUNCE_TIMEOUT) + break; + msleep(HUB_DEBOUNCE_STEP); + } + + dev_dbg(&port_dev->dev, "debounce total %dms stable %dms status 0x%x\n", + total_time, stable_time, portstatus); + + if (stable_time < HUB_DEBOUNCE_STABLE) + return -ETIMEDOUT; + return portstatus; +} +``` +In this function, we can clearly see that , it calls another function ```hub_port_status``` with reference of portstatus: +``` ret = hub_port_status(hub, port1, &portstatus, &portchange); ``` +So hub_port_status defined in the same file line 602 as following: +```C +static int hub_port_status(struct usb_hub *hub, int port1, + u16 *status, u16 *change) +{ + return hub_ext_port_status(hub, port1, HUB_PORT_STATUS, + status, change, NULL); +} +``` +So finally, lets examine at ```hub_ext_port_status``` code, which is defined in line 573: +``` +static int hub_ext_port_status(struct usb_hub *hub, int port1, int type, + u16 *status, u16 *change, u32 *ext_status) +{ + int ret; + int len = 4; + + if (type != HUB_PORT_STATUS) + len = 8; + + mutex_lock(&hub->status_mutex); + ret = get_port_status(hub->hdev, port1, &hub->status->port, type, len); + if (ret < len) { + if (ret != -ENODEV) + dev_err(hub->intfdev, + "%s failed (err = %d)\n", __func__, ret); + if (ret >= 0) + ret = -EIO; + } else { + *status = le16_to_cpu(hub->status->port.wPortStatus); + *change = le16_to_cpu(hub->status->port.wPortChange); + if (type != HUB_PORT_STATUS && ext_status) + *ext_status = le32_to_cpu( + hub->status->port.dwExtPortStatus); + ret = 0; + } + mutex_unlock(&hub->status_mutex); + return ret; +} +``` +In else part, there is assignment to status value : +``` *status = le16_to_cpu(hub->status->port.wPortStatus); ``` +So in my opinion, Infer created a false-positive warning for this case. + + diff --git a/reports/v416/infer-014/tools:lib:subcmd:help:c:nulldereference:0001.md b/reports/v416/infer-014/tools:lib:subcmd:help:c:nulldereference:0001.md new file mode 100644 index 0000000..e15837d --- /dev/null +++ b/reports/v416/infer-014/tools:lib:subcmd:help:c:nulldereference:0001.md @@ -0,0 +1,50 @@ +# Analysis Report 0001 # + +## General ## +**Warning Type:** NULL_DEREFERENCE +**Warning Explanation:** Pointer `ent` last assigned on line 18 could be null and is dereferenced at line 20, column 2. +```C + struct cmdname *ent = malloc(sizeof(*ent) + len + 1); + + ent->len = len; + memcpy(ent->name, name, len); + ent->name[len] = 0; +``` +**File Location:** tools/lib/subcmd/help.c:20 +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- + +## Manuel Assesment ## +**Classification:** POSITIVE, There may be an error during runtime +### Rationale ### +It is very clear that, If malloc function fails, then there will be a segmentation error at line 20: +```C +void add_cmdname(struct cmdnames *cmds, const char *name, size_t len) +{ + struct cmdname *ent = malloc(sizeof(*ent) + len + 1); + + ent->len = len; + memcpy(ent->name, name, len); + ent->name[len] = 0; + + ALLOC_GROW(cmds->names, cmds->cnt + 1, cmds->alloc); + cmds->names[cmds->cnt++] = ent; +} +``` +**Resolution:** +We can simply add an if line after malloc, and check if ent is null or not: +```C +void add_cmdname(struct cmdnames *cmds, const char *name, size_t len) +{ + struct cmdname *ent = malloc(sizeof(*ent) + len + 1); + if(!ent) exit 1; + ent->len = len; + memcpy(ent->name, name, len); + ent->name[len] = 0; + + ALLOC_GROW(cmds->names, cmds->cnt + 1, cmds->alloc); + cmds->names[cmds->cnt++] = ent; +} +``` diff --git a/reports/v416/infer-014/x86:events:intel:bts:c:nulldereference:0002.md b/reports/v416/infer-014/x86:events:intel:bts:c:nulldereference:0002.md new file mode 100644 index 0000000..212a60e --- /dev/null +++ b/reports/v416/infer-014/x86:events:intel:bts:c:nulldereference:0002.md @@ -0,0 +1,97 @@ +# Analysis Report 0002 # + +## General ## +**Warning Type:** NULL_DEREFERENCE +**Warning Explanation:** Pointer `buf` last assigned on line 229 could be null and is dereferenced at line 232, column 7. +```C + u64 config = 0; + + if (!buf->snapshot) + config |= ARCH_PERFMON_EVENTSEL_INT; + if (!event->attr.exclude_kernel) +``` +**File Location:** arch/x86/events/intel/bts.c:232: +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- + +## Manuel Assesment ## +**Classification:** UNDECIDED +### Rationale ### +In here, *buf pointer points to a memory address that returned from perf_get_aux(&bts->handle) function. +```C +static void __bts_event_start(struct perf_event *event) +{ + struct bts_ctx *bts = this_cpu_ptr(&bts_ctx); + struct bts_buffer *buf = perf_get_aux(&bts->handle); + u64 config = 0; + + if (!buf->snapshot) + config |= ARCH_PERFMON_EVENTSEL_INT; +``` +```C +struct bts_ctx { + struct perf_output_handle handle; + struct debug_store ds_back; + int state; +}; +``` +So lets look at perf_get_aux(struct perf_output_handle*) function definition: +It defined in kernel/events/ring_buffer.c file as: + +```C +void *perf_get_aux(struct perf_output_handle *handle) +{ + /* this is only valid between perf_aux_output_begin and *_end */ + if (!handle->event) + return NULL; + + return handle->rb->aux_priv; +} +``` +So that function is not a null-safe function. If ```bts->handle``` is pointing to an invalid memory address, then this function may fail. +To find out that, if it is possible, then we should look at ```this_cpu_ptr(&bts_ctx);``` line. In here ```bts_ctx``` is an global variable. +```this_cpu_ptr``` macro defined in [include/linux/percpu-defs.h](https://elixir.bootlin.com/linux/v4.16/source/include/linux/percpu-defs.h) file, as three different instances, which selected according to kernel-configuration. +```C +#define raw_cpu_ptr(ptr) \ +({ \ + __verify_pcpu_ptr(ptr); \ + arch_raw_cpu_ptr(ptr); \ +}) + +#ifdef CONFIG_DEBUG_PREEMPT +#define this_cpu_ptr(ptr) \ +({ \ + __verify_pcpu_ptr(ptr); \ + SHIFT_PERCPU_PTR(ptr, my_cpu_offset); \ +}) +#else +#define this_cpu_ptr(ptr) raw_cpu_ptr(ptr) +#endif + +#else /* CONFIG_SMP */ + +#define VERIFY_PERCPU_PTR(__p) \ +({ \ + __verify_pcpu_ptr(__p); \ + (typeof(*(__p)) __kernel __force *)(__p); \ +}) + +#define per_cpu_ptr(ptr, cpu) ({ (void)(cpu); VERIFY_PERCPU_PTR(ptr); }) +#define raw_cpu_ptr(ptr) per_cpu_ptr(ptr, 0) +#define this_cpu_ptr(ptr) raw_cpu_ptr(ptr) +``` +It is very hard to track this function since it has multiple definitions and also using a global variable, so I checked developers comment that written above ```__bts_event_start``` as: +```C +/* + * Ordering PMU callbacks wrt themselves and the PMI is done by means + * of bts::state, which: + * - is set when bts::handle::event is valid, that is, between + * perf_aux_output_begin() and perf_aux_output_end(); + * - is zero otherwise; + * - is ordered against bts::handle::event with a compiler barrier. + */ +``` +According to comment, that function is safe, however I couldn't really decided if infer is right or not + diff --git a/reports/v416/infer-014/x86:events:intel:p4:c:nulldereference:0003.md b/reports/v416/infer-014/x86:events:intel:p4:c:nulldereference:0003.md new file mode 100644 index 0000000..4512ff8 --- /dev/null +++ b/reports/v416/infer-014/x86:events:intel:p4:c:nulldereference:0003.md @@ -0,0 +1,121 @@ +# Analysis Report 0003 # + +## General ## +**Warning Type:** NULL_DEREFERENCE +**Warning Explanation:** pointer `bind` last assigned on line 723 could be null and is dereferenced at line 724, column 9. +```C + config = p4_general_events[hw_event]; + bind = p4_config_get_bind(config); + esel = P4_OPCODE_ESEL(bind->opcode); + config |= p4_config_pack_cccr(P4_CCCR_ESEL(esel)); + +``` +**File Location:** arch/x86/events/intel/p4.c:724: +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- + +## Manuel Assesment ## +**Classification:** +### Rationale ### +As Infer stated, there may be a problem in ```P4_OPCODE_ESEL(bind->opcode);``` call, if bind points to an invalid memory address. +```C +static u64 p4_pmu_event_map(int hw_event) +{ + struct p4_event_bind *bind; + unsigned int esel; + u64 config; + + config = p4_general_events[hw_event]; + bind = p4_config_get_bind(config); + esel = P4_OPCODE_ESEL(bind->opcode); + config |= p4_config_pack_cccr(P4_CCCR_ESEL(esel)); + + return config; +} +``` +Let's start with ```config = p4_general_events[hw_event]``` line to check ```config```'s possible values: +```p4_general_events``` is a huge u64 array as following: +```C +static u64 p4_general_events[PERF_COUNT_HW_MAX] = { + /* non-halted CPU clocks */ + [PERF_COUNT_HW_CPU_CYCLES] = + p4_config_pack_escr(P4_ESCR_EVENT(P4_EVENT_GLOBAL_POWER_EVENTS) | + P4_ESCR_EMASK_BIT(P4_EVENT_GLOBAL_POWER_EVENTS, RUNNING)) | + P4_CONFIG_ALIASABLE, + + /* + * retired instructions + * in a sake of simplicity we don't use the FSB tagging + */ + [PERF_COUNT_HW_INSTRUCTIONS] = + p4_config_pack_escr(P4_ESCR_EVENT(P4_EVENT_INSTR_RETIRED) | + P4_ESCR_EMASK_BIT(P4_EVENT_INSTR_RETIRED, NBOGUSNTAG) | + P4_ESCR_EMASK_BIT(P4_EVENT_INSTR_RETIRED, BOGUSNTAG)), + + /* cache hits */ + [PERF_COUNT_HW_CACHE_REFERENCES] = + p4_config_pack_escr(P4_ESCR_EVENT(P4_EVENT_BSQ_CACHE_REFERENCE) | + P4_ESCR_EMASK_BIT(P4_EVENT_BSQ_CACHE_REFERENCE, RD_2ndL_HITS) | + P4_ESCR_EMASK_BIT(P4_EVENT_BSQ_CACHE_REFERENCE, RD_2ndL_HITE) | + P4_ESCR_EMASK_BIT(P4_EVENT_BSQ_CACHE_REFERENCE, RD_2ndL_HITM) | + P4_ESCR_EMASK_BIT(P4_EVENT_BSQ_CACHE_REFERENCE, RD_3rdL_HITS) | + P4_ESCR_EMASK_BIT(P4_EVENT_BSQ_CACHE_REFERENCE, RD_3rdL_HITE) | + P4_ESCR_EMASK_BIT(P4_EVENT_BSQ_CACHE_REFERENCE, RD_3rdL_HITM)), + + /* cache misses */ + [PERF_COUNT_HW_CACHE_MISSES] = + p4_config_pack_escr(P4_ESCR_EVENT(P4_EVENT_BSQ_CACHE_REFERENCE) | + P4_ESCR_EMASK_BIT(P4_EVENT_BSQ_CACHE_REFERENCE, RD_2ndL_MISS) | + P4_ESCR_EMASK_BIT(P4_EVENT_BSQ_CACHE_REFERENCE, RD_3rdL_MISS) | + P4_ESCR_EMASK_BIT(P4_EVENT_BSQ_CACHE_REFERENCE, WR_2ndL_MISS)), + + /* branch instructions retired */ + [PERF_COUNT_HW_BRANCH_INSTRUCTIONS] = + p4_config_pack_escr(P4_ESCR_EVENT(P4_EVENT_RETIRED_BRANCH_TYPE) | + P4_ESCR_EMASK_BIT(P4_EVENT_RETIRED_BRANCH_TYPE, CONDITIONAL) | + P4_ESCR_EMASK_BIT(P4_EVENT_RETIRED_BRANCH_TYPE, CALL) | + P4_ESCR_EMASK_BIT(P4_EVENT_RETIRED_BRANCH_TYPE, RETURN) | + P4_ESCR_EMASK_BIT(P4_EVENT_RETIRED_BRANCH_TYPE, INDIRECT)), + + /* mispredicted branches retired */ + [PERF_COUNT_HW_BRANCH_MISSES] = + p4_config_pack_escr(P4_ESCR_EVENT(P4_EVENT_MISPRED_BRANCH_RETIRED) | + P4_ESCR_EMASK_BIT(P4_EVENT_MISPRED_BRANCH_RETIRED, NBOGUS)), + + /* bus ready clocks (cpu is driving #DRDY_DRV\#DRDY_OWN): */ + [PERF_COUNT_HW_BUS_CYCLES] = + p4_config_pack_escr(P4_ESCR_EVENT(P4_EVENT_FSB_DATA_ACTIVITY) | + P4_ESCR_EMASK_BIT(P4_EVENT_FSB_DATA_ACTIVITY, DRDY_DRV) | + P4_ESCR_EMASK_BIT(P4_EVENT_FSB_DATA_ACTIVITY, DRDY_OWN)) | + p4_config_pack_cccr(P4_CCCR_EDGE | P4_CCCR_COMPARE), +}; +``` +Then continue with ```p4_config_get_bind(config)``` function +```C + +static struct p4_event_bind *p4_config_get_bind(u64 config) +{ + unsigned int evnt = p4_config_unpack_event(config); + struct p4_event_bind *bind = NULL; + + if (evnt < ARRAY_SIZE(p4_event_bind_map)) + bind = &p4_event_bind_map[evnt]; + + return bind; +} +``` +As you can see, if ```evnt > ARRAY_SIZE(p4_event_bind_map)```, then bind will be returned as a ```NULL``` value. +However to learn if it is possible during runtime or not, we must find ```evnt```'s possible values and length of ```ARRAY_SIZE(p4_event_bind_map)``` +--> TODO Look Later more detailed + + + + + + + + + + diff --git a/reports/v416/infer-015/drivers:dma-buf:reservation:c:uninitialized:0005.md b/reports/v416/infer-015/drivers:dma-buf:reservation:c:uninitialized:0005.md new file mode 100644 index 0000000..669fa2d --- /dev/null +++ b/reports/v416/infer-015/drivers:dma-buf:reservation:c:uninitialized:0005.md @@ -0,0 +1,86 @@ +# Analysis Report 0005 # + +## General ## +**Warning Type:** UNINITIALIZED_VALUE +**Warning Explanation:** The value read from k was never initialized. +```C +/* Drop the references to the signaled fences */ +for (i = k; i < fobj->shared_max; ++i) { + struct dma_fence *f; +} +``` +**File Location:** drivers/dma-buf/reservation.c:207 +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- + +## Manuel Assesment ## +**Classification:** False - Positive +### Rationale ### +```C +static void +reservation_object_add_shared_replace(struct reservation_object *obj, + struct reservation_object_list *old, + struct reservation_object_list *fobj, + struct dma_fence *fence) +{ + unsigned i, j, k; +... + + if (!old) { + RCU_INIT_POINTER(fobj->shared[0], fence); + fobj->shared_count = 1; + goto done; + } + + /* + * no need to bump fence refcounts, rcu_read access + * requires the use of kref_get_unless_zero, and the + * references from the old struct are carried over to + * the new. + */ + for (i = 0, j = 0, k = fobj->shared_max; i < old->shared_count; ++i) { + struct dma_fence *check; + + check = rcu_dereference_protected(old->shared[i], + reservation_object_held(obj)); + + if (check->context == fence->context || + dma_fence_is_signaled(check)) + RCU_INIT_POINTER(fobj->shared[--k], check); + else + RCU_INIT_POINTER(fobj->shared[j++], check); + } + fobj->shared_count = j; + RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); + fobj->shared_count++; + +done: + preempt_disable(); + write_seqcount_begin(&obj->seq); + /* + * RCU_INIT_POINTER can be used here, + * seqcount provides the necessary barriers + */ + RCU_INIT_POINTER(obj->fence, fobj); + write_seqcount_end(&obj->seq); + preempt_enable(); + + if (!old) + return; + + /* Drop the references to the signaled fences */ + for (i = k; i < fobj->shared_max; ++i) { + struct dma_fence *f; + + f = rcu_dereference_protected(fobj->shared[i], + reservation_object_held(obj)); + dma_fence_put(f); + } + kfree_rcu(old, rcu); +} + +``` +In this function, there is no assignment made to k, if function follows ```goto done``` path. However in this case, after the second ```if (!old)``` check, ```reservation_object_add_shared_replace``` will return immediately, without entering loop. So I think its a false-positive warning. + diff --git a/reports/v416/infer-015/drivers:hid:hid-ntrig:c:deadstorage:0004.md b/reports/v416/infer-015/drivers:hid:hid-ntrig:c:deadstorage:0004.md new file mode 100644 index 0000000..63bdcc4 --- /dev/null +++ b/reports/v416/infer-015/drivers:hid:hid-ntrig:c:deadstorage:0004.md @@ -0,0 +1,52 @@ +# Analysis Report 0004 # + +## General ## +**Warning Type:** DEAD_STORAGE +**Warning Explanation:** The value written to &ret (type int) is never used. +```C + +if (ret == 8) { +> ret = ntrig_version_string(&data[2], buf); + hid_info(hdev, "Firmware version: %s (%02x%02x %02x%02x)\n", + +} +``` +**File Location:** drivers/hid/hid-ntrig.c:162 +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- + +## Manuel Assesment ## +**Classification:** POSITIVE +### Rationale ### +```C +static void ntrig_report_version(struct hid_device *hdev) +{ + int ret; + char buf[20]; + struct usb_device *usb_dev = hid_to_usb_dev(hdev); + unsigned char *data = kmalloc(8, GFP_KERNEL); + + if (!data) + goto err_free; + + ret = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0), + USB_REQ_CLEAR_FEATURE, + USB_TYPE_CLASS | USB_RECIP_INTERFACE | + USB_DIR_IN, + 0x30c, 1, data, 8, + USB_CTRL_SET_TIMEOUT); + + if (ret == 8) { + ret = ntrig_version_string(&data[2], buf); + + hid_info(hdev, "Firmware version: %s (%02x%02x %02x%02x)\n", + buf, data[2], data[3], data[4], data[5]); + } + +err_free: + kfree(data); +} +``` +As infer stated, ```ret``` value is not used after ```ret = ntrig_version_string(&data[2], buf);```. No need to assign return value of ```ntrig_version_string``` function to ```ret```. diff --git a/reports/v416/infer-015/drivers:rtc:rtc-dev:c:uninitialized:0003.md b/reports/v416/infer-015/drivers:rtc:rtc-dev:c:uninitialized:0003.md new file mode 100644 index 0000000..255a134 --- /dev/null +++ b/reports/v416/infer-015/drivers:rtc:rtc-dev:c:uninitialized:0003.md @@ -0,0 +1,72 @@ +# Analysis Report 0003 # + +## General ## +**Warning Type:** UNINITIALIZED_VALUE +**Warning Explanation:** The value read from ret was never initialized. +```C + sizeof(unsigned long); + } +return ret; +} +``` +**File Location:** drivers/rtc/rtc-dev.c:194 +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- +**Similar-Case:** [FalsePositive-Uninitialized-loop-problem](https://github.com/OzanAlpay/linux-kernel-analysis/tree/infer-documentation/infer/MockCodes/infer-uninitialized-with-loop) +## Manuel Assesment ## +**Classification:** FALSE-POSITIVE +### Rationale ### +```C +... + ssize_t ret; + + if (count != sizeof(unsigned int) && count < sizeof(unsigned long)) + return -EINVAL; + + add_wait_queue(&rtc->irq_queue, &wait); + do { + __set_current_state(TASK_INTERRUPTIBLE); + + spin_lock_irq(&rtc->irq_lock); + data = rtc->irq_data; + rtc->irq_data = 0; + spin_unlock_irq(&rtc->irq_lock); + + if (data != 0) { + ret = 0; + break; + } + if (file->f_flags & O_NONBLOCK) { + ret = -EAGAIN; + break; + } + if (signal_pending(current)) { + ret = -ERESTARTSYS; + break; + } + schedule(); + } while (1); + set_current_state(TASK_RUNNING); + remove_wait_queue(&rtc->irq_queue, &wait); + + if (ret == 0) { + /* Check for any data updates */ + if (rtc->ops->read_callback) + data = rtc->ops->read_callback(rtc->dev.parent, + data); + + if (sizeof(int) != sizeof(long) && + count == sizeof(unsigned int)) + ret = put_user(data, (unsigned int __user *)buf) ?: + sizeof(unsigned int); + else + ret = put_user(data, (unsigned long __user *)buf) ?: + sizeof(unsigned long); + } + return ret; +} +``` +In the function above, there is an infinite loop, and without assigning a value to ```ret``` value , it cannot be broken. +In my opinion ```ret``` cannot have an uninitialized before return. diff --git a/reports/v416/infer-015/drivers:usb:core:devio:c:uninitiliazed:0001.md b/reports/v416/infer-015/drivers:usb:core:devio:c:uninitiliazed:0001.md new file mode 100644 index 0000000..e230b1a --- /dev/null +++ b/reports/v416/infer-015/drivers:usb:core:devio:c:uninitiliazed:0001.md @@ -0,0 +1,50 @@ +# Analysis Report 0001 # + +## General ## +**Warning Type:** UNINITIALIZED_VALUE +**Warning Explanation:** The value read from gd.interface was never initialized. +```C + if (copy_from_user(&gd, arg, sizeof(gd))) + return -EFAULT; + intf = usb_ifnum_to_if(ps->dev, gd.interface); + if (!intf || !intf->dev.driver) + +``` +**File Location:** drivers/usb/core/devio.c:1291 +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- + +## Manuel Assesment ## +**Classification:** FALSE-POSITIVE +### Rationale ### +```C +static int proc_getdriver(struct usb_dev_state *ps, void __user *arg) +{ + struct usbdevfs_getdriver gd; + struct usb_interface *intf; + int ret; + + if (copy_from_user(&gd, arg, sizeof(gd))) + return -EFAULT; + intf = usb_ifnum_to_if(ps->dev, gd.interface); + if (!intf || !intf->dev.driver) + ret = -ENODATA; + else { + strlcpy(gd.driver, intf->dev.driver->name, + sizeof(gd.driver)); + ret = (copy_to_user(arg, &gd, sizeof(gd)) ? -EFAULT : 0); + } + return ret; +} +``` +In this function, at ```if (copy_from_user(&gd, arg, sizeof(gd)))``` block, programmer tried to get data from user space.(see)[https://www.fsl.cs.sunysb.edu/kernel-api/re257.html], and returned an ```-EFAULT``` in copy failure case. Infer warns us about in ```intf = usb_ifnum_to_if(ps->dev, gd.interface);``` assignment, we may pass an uinit gd.interface value. So we must look at usbdevfs_getdriver struct to analyze this case better. +```C +struct usbdevfs_getdriver { + unsigned int interface; + char driver[USBDEVFS_MAXDRIVERNAME + 1]; +}; +``` +Since copy_from_user function returned without any error, and there isn't any warning in infer for ```include/linux/uaccess.h:copy_from_user``` function flow. I guess interface and driver values are set and they are not uninitialized. So in my opinion it is a false-positive warning. + diff --git a/reports/v416/infer-015/kernel:sched:fair:c:deadstorage:0007.md b/reports/v416/infer-015/kernel:sched:fair:c:deadstorage:0007.md new file mode 100644 index 0000000..37edd65 --- /dev/null +++ b/reports/v416/infer-015/kernel:sched:fair:c:deadstorage:0007.md @@ -0,0 +1,59 @@ +# Analysis Report 0007 # + +## General ## +**Warning Type:** DEAD_STORE +**Warning Explanation:** The value written to &now (type unsigned long) is never used. +```C +static inline bool nohz_kick_needed(struct rq *rq) +{ + unsigned long now = jiffies; + struct sched_domain_shared *sds; + struct sched_domain *sd; +``` +**File Location:** kernel/sched/fair.c:9312 +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- +**Similar Case:** +## Manuel Assesment ## +**Classification:** False - Positive +### Rationale ### +```C +static inline bool nohz_kick_needed(struct rq *rq) +{ + unsigned long now = jiffies; + struct sched_domain_shared *sds; + struct sched_domain *sd; + int nr_busy, i, cpu = rq->cpu; + bool kick = false; + + if (unlikely(rq->idle_balance)) + return false; + + /* + * We may be recently in ticked or tickless idle mode. At the first + * busy tick after returning from idle, we will update the busy stats. + */ + set_cpu_sd_state_busy(); + nohz_balance_exit_idle(cpu); + + /* + * None are in tickless mode and hence no need for NOHZ idle load + * balancing. + */ + if (likely(!atomic_read(&nohz.nr_cpus))) + return false; + + if (time_before(now, nohz.next_balance)) + return false; +``` +As we can observe in here, it is very obvious that ```unsigned long now``` value passed to ```time_before(now, nohz.next_balance)```. This function defined as a macro in ```include/linux/jiffies.h``` +```C +#define time_after(a,b) \ + (typecheck(unsigned long, a) && \ + typecheck(unsigned long, b) && \ + ((long)((b) - (a)) < 0)) +#define time_before(a,b) time_after(b,a) +``` +So thats a very obvious false-positive warning, and its suprising that Infer created a warning in this case diff --git a/reports/v416/infer-015/lib:decompress-bunzip2:c:uninitialized:0006.md b/reports/v416/infer-015/lib:decompress-bunzip2:c:uninitialized:0006.md new file mode 100644 index 0000000..a88144b --- /dev/null +++ b/reports/v416/infer-015/lib:decompress-bunzip2:c:uninitialized:0006.md @@ -0,0 +1,173 @@ +# Analysis Report 0006 # + +## General ## +**Warning Type:** UNINITIALIZED_VALUE +**Warning Explanation:** The value read from length[_] was never initialized. +```C +} + /* Find largest and smallest lengths in this group */ + minLen = maxLen = length[0]; + for (i = 1; i < symCount; i++) { +``` +**File Location:** lib/decompress_bunzip2.c:271 +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- +**Similar Case:** [FalsePositive-Uninitialized-loop-problem](https://github.com/OzanAlpay/linux-kernel-analysis/tree/infer-documentation/infer/MockCodes/infer-uninitialized-with-loop) +## Manuel Assesment ## +**Classification:** False - Positive +### Rationale ### +Since it is a huge function, I will examine it part by part. +```C +static int INIT get_next_block(struct bunzip_data *bd) +{ + struct group_data *hufGroup = NULL; + int *base = NULL; + int *limit = NULL; + int dbufCount, nextSym, dbufSize, groupCount, selector, + i, j, k, t, runPos, symCount, symTotal, nSelectors, *byteCount; + unsigned char uc, *symToByte, *mtfSymbol, *selectors; + unsigned int *dbuf, origPtr; + +.... + + symTotal = 0; +``` +In here symTotal assigned to 0 +```C + for (i = 0; i < 16; i++) { + if (t&(1 << (15-i))) { + k = get_bits(bd, 16); + for (j = 0; j < 16; j++) + if (k&(1 << (15-j))) + symToByte[symTotal++] = (16*i)+j; + } + } +``` +Depends on bunzip data value, symTotal's value may be increase, but at worst case it is still 0 +```C + /* How many different Huffman coding groups does this block use? */ + groupCount = get_bits(bd, 3); + if (groupCount < 2 || groupCount > MAX_GROUPS) + return RETVAL_DATA_ERROR; + /* nSelectors: Every GROUP_SIZE many symbols we select a new + Huffman coding group. Read in the group selector list, + which is stored as MTF encoded bit runs. (MTF = Move To + Front, as each value is used it's moved to the start of the + list.) */ + nSelectors = get_bits(bd, 15); + if (!nSelectors) + return RETVAL_DATA_ERROR; + for (i = 0; i < groupCount; i++) + mtfSymbol[i] = i; + for (i = 0; i < nSelectors; i++) { + /* Get next value */ + for (j = 0; get_bits(bd, 1); j++) + if (j >= groupCount) + return RETVAL_DATA_ERROR; + /* Decode MTF to get the next selector */ + uc = mtfSymbol[j]; + for (; j; j--) + mtfSymbol[j] = mtfSymbol[j-1]; + mtfSymbol[0] = selectors[i] = uc; + } + /* Read the Huffman coding tables for each group, which code + for symTotal literal symbols, plus two run symbols (RUNA, + RUNB) */ + symCount = symTotal+2; +``` +In here programmer assigned symCount value to symTotal+2, so in worst case it is 2 +```C + for (j = 0; j < groupCount; j++) { + unsigned char length[MAX_SYMBOLS], temp[MAX_HUFCODE_BITS+1]; + int minLen, maxLen, pp; + /* Read Huffman code lengths for each symbol. They're + stored in a way similar to mtf; record a starting + value for the first symbol, and an offset from the + previous value for everys symbol after that. + (Subtracting 1 before the loop and then adding it + back at the end is an optimization that makes the + test inside the loop simpler: symbol length 0 + becomes negative, so an unsigned inequality catches + it.) */ + t = get_bits(bd, 5)-1; +``` +Programmer assign a value to t in here. +```C + for (i = 0; i < symCount; i++) { + for (;;) { + if (((unsigned)t) > (MAX_HUFCODE_BITS-1)) + return RETVAL_DATA_ERROR; + + /* If first bit is 0, stop. Else + second bit indicates whether to + increment or decrement the value. + Optimization: grab 2 bits and unget + the second if the first was 0. */ + + k = get_bits(bd, 2); + if (k < 2) { + bd->inbufBitCount++; + break; + } + /* Add one if second bit 1, else + * subtract 1. Avoids if/else */ + t += (((k+1)&2)-1); +``` +There may be an update to value of t, but it depends.(if k<2 then there won't be any update) +```C + } + /* Correct for the initial -1, to get the + * final symbol length */ + length[i] = t+1; + } + /* Find largest and smallest lengths in this group */ + minLen = maxLen = length[0]; +``` +In here, we know that symCount at least equal to 2. So in this case it will assign values to ```length[0] and length[1]```. So lets look at value of ```t```. If it is not an uninitialized value, then this function is safe. To understand it better, we must check ```get_bits()``` function. +```C +/* Return the next nnn bits of input. All reads from the compressed input + are done through this function. All reads are big endian */ +static unsigned int INIT get_bits(struct bunzip_data *bd, char bits_wanted) +{ + unsigned int bits = 0; + + /* If we need to get more data from the byte buffer, do so. + (Loop getting one byte at a time to enforce endianness and avoid + unaligned access.) */ + while (bd->inbufBitCount < bits_wanted) { + /* If we need to read more data from file into byte buffer, do + so */ + if (bd->inbufPos == bd->inbufCount) { + if (bd->io_error) + return 0; + bd->inbufCount = bd->fill(bd->inbuf, BZIP2_IOBUF_SIZE); + if (bd->inbufCount <= 0) { + bd->io_error = RETVAL_UNEXPECTED_INPUT_EOF; + return 0; + } + bd->inbufPos = 0; + } + /* Avoid 32-bit overflow (dump bit buffer to top of output) */ + if (bd->inbufBitCount >= 24) { + bits = bd->inbufBits&((1 << bd->inbufBitCount)-1); + bits_wanted -= bd->inbufBitCount; + bits <<= bits_wanted; + bd->inbufBitCount = 0; + } + /* Grab next 8 bits of input from buffer. */ + bd->inbufBits = (bd->inbufBits << 8)|bd->inbuf[bd->inbufPos++]; + bd->inbufBitCount += 8; + } + /* Calculate result */ + bd->inbufBitCount -= bits_wanted; + bits |= (bd->inbufBits >> bd->inbufBitCount)&((1 << bits_wanted)-1); + + return bits; +} +``` +That function returns an ```unsigned int bits```. Programmer assigned ```bits``` value at the beginning of function. I can't find any dangerous part of code in that function, that may return an uninitialized value +So in my opinion in any case ```length[0]``` will be initialized, before ``` minLen = maxLen = length[0];``` line called. In my opinion since it will assign a value to ```length[0]``` inside a loop, Infer creates a false-positive warning in this case. + + diff --git a/reports/v416/infer-015/usr:gen:init:cpio:c:resourceleak:0002.md b/reports/v416/infer-015/usr:gen:init:cpio:c:resourceleak:0002.md new file mode 100644 index 0000000..7737736 --- /dev/null +++ b/reports/v416/infer-015/usr:gen:init:cpio:c:resourceleak:0002.md @@ -0,0 +1,100 @@ +# Analysis Report 0002 # + +## General ## +**Warning Type:** RESOURCE_LEAK +**Warning Explanation:** +resource of type _IO_FILE acquired by call to ```fopen()``` at line 561, column 25 is not released after line 568, column 9. +```C +while (fgets(line, LINE_SIZE, cpio_list)) { + int type_idx; + size_t slen = strlen(line); +``` +resource of type _IO_FILE acquired by call to ```fopen()``` at line 561, column 25 is not released after line 580, column 12. +```C +if (! (type = strtok(line, " \t"))) { + fprintf(stderr, + "ERROR: incorrect format, could not locate file type line %d: '%s'\n", + line_nr, line); +``` +**File Location:** usr/gen_init_cpio.c:568 , usr/gen_init_cpio.c:580 +## History ## +**Introduced By:** TODO +**Reported Since:** TODO +**Resolved By:** -- + +## Manuel Assesment ## +**Classification:** FALSE-POSITIVE +### Rationale ### +Since these two warnings are closely related with each other, I want to investigate both of them together. +```C +filename = argv[optind]; +if (!strcmp(filename, "-")) + cpio_list = stdin; +else if (!(cpio_list = fopen(filename, "r"))) { + fprintf(stderr, "ERROR: unable to open '%s': %s\n\n", + filename, strerror(errno)); + usage(argv[0]); + exit(1); +} +``` +It is very clear that, programmer opens a file with ```fopen(filename, "r"))``` function and then start reading it line by line inside a while loop: +```C +while (fgets(line, LINE_SIZE, cpio_list)) { + int type_idx; + size_t slen = strlen(line); + + line_nr++; + + if ('#' == *line) { + /* comment - skip to next line */ + continue; + } + + if (! (type = strtok(line, " \t"))) { + fprintf(stderr, + "ERROR: incorrect format, could not locate file type line %d: '%s'\n", + line_nr, line); + ec = -1; + break; + } + + if ('\n' == *type) { + /* a blank line */ + continue; + } + + if (slen == strlen(type)) { + /* must be an empty line */ + continue; + } + + if (! (args = strtok(NULL, "\n"))) { + fprintf(stderr, + "ERROR: incorrect format, newline required line %d: '%s'\n", + line_nr, line); + ec = -1; + } + + for (type_idx = 0; file_handler_table[type_idx].type; type_idx++) { + int rc; + if (! strcmp(line, file_handler_table[type_idx].type)) { + if ((rc = file_handler_table[type_idx].handler(args))) { + ec = rc; + fprintf(stderr, " line %d\n", line_nr); + } + break; + } + } + + if (NULL == file_handler_table[type_idx].type) { + fprintf(stderr, "unknown file type line %d: '%s'\n", + line_nr, line); + } +} +if (ec == 0) + cpio_trailer(); + +exit(ec); +``` +However in any case, this function will execute ```exit(ec)``` command, and that command will close all opened streams. +